Correctifs appliqués

Joe Conway pushed:

Heikki Linnakangas pushed:

  • Document the "replication" option in StartupMessage. It is documented in the Streaming Replication Protocol section, but was missing from the list of options in StartupMessage description.
  • Fix indentation. Oops, I forgot to "git add" this to previous commit.
  • Minor cleanup of backend SCRAM code. Free each SASL message after sending it. It's not a lot of wasted memory, and it's short-lived, but the authentication code in general tries to pfree() stuff, so let's follow the example. Adding the pfree() revealed a little bug in build_server_first_message(). It attempts to keeps a copy of the sent message, but it was missing a pstrdup(), so the pointer started to dangle, after adding the pfree() into CheckSCRAMAuth(). Reword comments and debug messages slightly, while we're at it. Reviewed by Michael Paquier. Discussion:
  • Improve the SASL authentication protocol. This contains some protocol changes to SASL authentiation (which is new in v10): * For future-proofing, in the AuthenticationSASL message that begins SASL authentication, provide a list of SASL mechanisms that the server supports, for the client to choose from. Currently, it's always just SCRAM-SHA-256. * Add a separate authentication message type for the final server->client SASL message, which the client doesn't need to respond to. This makes it unambiguous whether the client is supposed to send a response or not. The SASL mechanism should know that anyway, but better to be explicit. Also, in the server, support clients that don't send an Initial Client response in the first SASLInitialResponse message. The server is supposed to first send an empty request in that case, to which the client will respond with the data that usually comes in the Initial Client Response. libpq uses the Initial Client Response field and doesn't need this, and I would assume any other sensible implementation to use Initial Client Response, too, but let's follow the SASL spec. Improve the documentation on SASL authentication in protocol. Add a section describing the SASL message flow, and some details on our SCRAM-SHA-256 implementation. Document the different kinds of PasswordMessages that the frontend sends in different phases of SASL authentication, as well as GSS/SSPI authentication as separate message formats. Even though they're all 'p' messages, and the exact format depends on the context, describing them as separate message formats makes the documentation more clear. Reviewed by Michael Paquier and Álvaro Hernández Tortosa. Discussion:
  • Refactor libpq authentication request processing. Move the responsibility of reading the data from the authentication request message from PQconnectPoll() to pg_fe_sendauth(). This way, PQconnectPoll() doesn't need to know about all the different authentication request types, and we don't need the extra fields in the pg_conn struct to pass the data from PQconnectPoll() to pg_fe_sendauth() anymore. Reviewed by Michael Paquier. Discussion:

Tom Lane pushed:

  • Move isolationtester's is-blocked query into C code for speed. Commit 4deb41381 modified isolationtester's query to see whether a session is blocked to also check for waits occurring in GetSafeSnapshot. However, it did that in a way that enormously increased the query's runtime under CLOBBER_CACHE_ALWAYS, causing the buildfarm members that use that to run about four times slower than before, and in some cases fail entirely. To fix, push the entire logic into a dedicated backend function. This should actually reduce the CLOBBER_CACHE_ALWAYS runtime from what it was previously, though I've not checked that. In passing, expose a SQL function to check for safe-snapshot blockage, comparable to pg_blocking_pids. This is more or less free given the infrastructure built to solve the other problem, so we might as well. Thomas Munro Discussion:
  • Improve castNode notation by introducing list-extraction-specific variants. This extends the castNode() notation introduced by commit 5bcab1114 to provide, in one step, extraction of a list cell's pointer and coercion to a concrete node type. For example, "lfirst_node(Foo, lc)" is the same as "castNode(Foo, lfirst(lc))". Almost half of the uses of castNode that have appeared so far include a list extraction call, so this is pretty widely useful, and it saves a few more keystrokes compared to the old way. As with the previous patch, back-patch the addition of these macros to pg_list.h, so that the notation will be available when back-patching. Patch by me, after an idea of Andrew Gierth's. Discussion:
  • Fix pgbench's --progress-timestamp option to print Unix-epoch timestamps. As a consequence of commit 1d63f7d2d, on platforms with CLOCK_MONOTONIC, you got some random timescale or other instead of standard Unix timestamps as expected. I'd attempted to fix pgbench for that change in commits 74baa1e3b and 67a875355, but missed this place. Fix in the same way as those previous commits, ie, just eat the cost of an extra gettimeofday(); one extra syscall per progress report isn't worth sweating over. Per report from Jeff Janes. In passing, use snprintf not sprintf for this purpose. I don't think there's any chance of actual buffer overrun, but it just looks safer. Discussion:
  • Handle restriction clause lists more uniformly in postgres_fdw. Clauses in the lists retained by postgres_fdw during planning were sometimes bare boolean clauses, sometimes RestrictInfos, and sometimes a mixture of the two in the same list. The comment about that situation didn't come close to telling the full truth, either. Aside from being confusing, this had a couple of bad practical consequences: * waste of planning cycles due to inability to cache per-clause selectivity and cost estimates; * sometimes, RestrictInfos would sneak into the fdw_private list of a finished Plan node, causing failures if, for example, we tried to ship the Plan tree to a parallel worker. (It may well be that it's a bug in the parallel-query logic that we would ever try to ship such a plan to a parallel worker, but in any case this deserves to be cleaned up.) To fix, rearrange so that clause lists in PgFdwRelationInfo are always lists of RestrictInfos, and then strip the RestrictInfos at the last minute when making a Plan node. In passing do a bit of refactoring and comment cleanup in postgresGetForeignPlan and foreign_join_ok. Although the messiness here dates back at least to 9.6, there's no evidence that it causes anything worse than wasted planning cycles in 9.6, so no back-patch for now. Per fuzz testing by Andreas Seltenreich. Tom Lane and Ashutosh Bapat Discussion:
  • Simplify handling of remote-qual pass-forward in postgres_fdw. Commit 0bf3ae88a encountered a need to pass the finally chosen remote qual conditions forward from postgresGetForeignPlan to postgresPlanDirectModify. It solved that by sticking them into the plan node's fdw_private list, which in hindsight was a pretty bad idea. In the first place, there's no use for those qual trees either in EXPLAIN or execution; indeed they could never safely be used for any post-planning purposes, because they would not get processed by setrefs.c. So they're just dead weight to carry around in the finished plan tree, plus being an attractive nuisance for somebody who might get the idea that they could be used that way. Secondly, because those qual trees (sometimes) contained RestrictInfos, they created a plan-transmission hazard for parallel query, which is how come we noticed a problem. We dealt with that symptom in commit 28b047875, but really a more straightforward and more efficient fix is to pass the data through in a new field of struct PgFdwRelationInfo. So do it that way. (There's no need to revert 28b047875, as it has sufficient reason to live anyway.) Per fuzz testing by Andreas Seltenreich. Discussion:
  • Remove bogus redefinition of _MSC_VER. Commit a4777f355 was a shade too mechanical: we don't want to override MSVC's own definition of _MSC_VER, as that breaks tests on its numerical value. Per buildfarm.
  • Mark finished Plan nodes with parallel_safe flags. We'd managed to avoid doing this so far, but it seems pretty obvious that it would be forced on us some day, and this is much the cleanest way of approaching the open problem that parallel-unsafe subplans are being transmitted to parallel workers. Anyway there's no space cost due to alignment considerations, and the time cost is pretty minimal since we're just copying the flag from the corresponding Path node. (At least in most cases ... some of the klugier spots in createplan.c have to work a bit harder.) In principle we could perhaps get rid of SubPlan.parallel_safe, but I thought it better to keep that in case there are reasons to consider a SubPlan unsafe even when its child plan is parallel-safe. This patch doesn't actually do anything with the new flags, but I thought I'd commit it separately anyway. Note: although this touches outfuncs/readfuncs, there's no need for a catversion bump because Plan trees aren't stored on disk. Discussion:
  • Avoid transferring parallel-unsafe subplans to parallel workers. Commit 5e6d8d2bb allowed parallel workers to execute parallel-safe subplans, but it transmitted the query's entire list of subplans to the worker(s). Since execMain.c blindly does ExecInitNode and later ExecEndNode on every list element, this resulted in parallel-unsafe plan nodes nonetheless getting started up and shut down in parallel workers. That seems mostly harmless as far as core plan node types go (but maybe not so much for Gather?). But it resulted in postgres_fdw opening and then closing extra remote connections, and it's likely that other non-parallel-safe FDWs or custom scan providers would have worse reactions. To fix, just make ExecSerializePlan replace parallel-unsafe subplans with NULLs in the cut-down plan tree that it transmits to workers. This relies on ExecInitNode and ExecEndNode to do nothing on NULL input, but they do anyway. If anything else is touching the dropped subplans in a parallel worker, that would be a bug to be fixed. (This thus provides a strong guarantee that we won't try to do something with a parallel-unsafe subplan in a worker.) This is, I think, the last fix directly occasioned by Andreas Seltenreich's bug report of a few days ago. Tom Lane and Amit Kapila Discussion:
  • Speed up hash_index regression test. Commit f5ab0a14e made this test take substantially longer than it used to. With a bit more care, we can get the runtime back down while achieving the same, or even a bit better, code coverage. Mithun Cy Discussion:
  • Move bootstrap-time lookup of regproc OIDs into Formerly, the bootstrap backend looked up the OIDs corresponding to names in regproc catalog entries using brute-force searches of pg_proc. It was somewhat remarkable that that worked at all, since it was used while populating other pretty-fundamental catalogs like pg_operator. And it was also quite slow, and getting slower as pg_proc gets bigger. This patch moves the lookup work into, so that the values in postgres.bki for regproc columns are always numeric OIDs, an option that regprocin() already supported. Perl isn't the world's speediest language, so this about doubles the time needed to run (from 0.3 to 0.6 sec on my machine). But we only do that at most once per build. The time needed to run initdb drops significantly --- on my machine, initdb --no-sync goes from 1.8 to 1.3 seconds. So this is a small net win even for just one initdb per build, and it becomes quite a nice win for test sequences requiring many initdb runs. Strip out the now-dead code for brute-force catalog searching in regprocin. We'd also cargo-culted similar logic into regoperin and some (not all) of the other reg*in functions. That is all dead code too since we currently have no need to load such values during bootstrap. I removed it all, reasoning that if we ever need such functionality it'd be much better to do it in a similar way to this patch. There might be some simplifications possible in the backend now that regprocin doesn't require doing catalog reads so early in bootstrap. I've not looked into that, though. Andreas Karlsson, with some small adjustments by me Discussion:
  • Fix regexport.c to behave sanely with lookaround constraints. regexport.c thought it could just ignore LACON arcs, but the correct behavior is to treat them as satisfiable while consuming zero input (rather reminiscently of commit 9f1e642d5). Otherwise, the emitted simplified-NFA representation may contain no paths leading from initial to final state, which unsurprisingly confuses pg_trgm, as seen in bug #14623 from Jeff Janes. Since regexport's output representation has no concept of an arc that consumes zero input, recurse internally to find the next normal arc(s) after any LACON transitions. We'd be forced into changing that representation if a LACON could be the last arc reaching the final state, but fortunately the regex library never builds NFAs with such a configuration, so there always is a next normal arc. Back-patch to 9.3 where this logic was introduced. Discussion:
  • Further fix pg_trgm's extraction of trigrams from regular expressions. Commit 9e43e8714 turns out to have been insufficient: not only is it necessary to track tentative parent links while considering a set of arc removals, but it's necessary to track tentative flag additions as well. This is because we always merge arc target states into arc source states; therefore, when considering a merge of the final state with some other, it is the other state that will acquire a new TSTATE_FIN bit. If there's another arc for the same color trigram that would cause merging of that state with the initial state, we failed to recognize the problem. The test cases for the prior commit evidently only exercised situations where a tentative merge with the initial state occurs before one with the final state. If it goes the other way around, we'll happily merge the initial and final states, either producing a broken final graph that would never match anything, or triggering the Assert added by the prior commit. It's tempting to consider switching the merge direction when the merge involves the final state, but I lack the time to analyze that idea in detail. Instead just keep track of the flag changes that would result from proposed merges, in the same way that the prior commit tracked proposed parent links. Along the way, add some more debugging support, because I'm not entirely confident that this is the last bug here. And tweak matters so that the file uses small integers rather than pointer values to identify states; that makes it more readable if you're just eyeballing it rather than fooling with Graphviz. And rename a couple of identically named struct fields to reduce confusion. Per report from Corey Csuhta. Add a test case based on his example. (Note: this case does not trigger the bug under 9.3, apparently because its different measurement of costs causes it to stop merging states before it hits the failure. I spent some time trying to find a variant that would fail in 9.3, without success; but I'm sure such cases exist.) Like the previous patch, back-patch to 9.3 where this code was added. Report:
  • Use one transaction while reading postgres.bki, not one per line. AFAICT, the only actual benefit of closing a bootstrap transaction is to reclaim transient memory. We can do that a lot more cheaply by just doing a MemoryContextReset on a suitable context. This gets the runtime of the "bootstrap" phase of initdb down to the point where, at least by eyeball, it's quite negligible compared to the rest of the phases. Per discussion with Andres Freund. Discussion:
  • Avoid passing function pointers across process boundaries. We'd already recognized that we can't pass function pointers across process boundaries for functions in loadable modules, since a shared library could get loaded at different addresses in different processes. But actually the practice doesn't work for functions in the core backend either, if we're using EXEC_BACKEND. This is the cause of recent failures on buildfarm member culicidae. Switch to passing a string function name in all cases. Something like this needs to be back-patched into 9.6, but let's see if the buildfarm likes it first. Petr Jelinek, with a bunch of basically-cosmetic adjustments by me Discussion:
  • More cleanup of manipulations of hash indexes' hasho_flag field. Not much point in defining test macros for the flag bits if we don't use 'em. Amit Kapila
  • Clean up manipulations of hash indexes' hasho_flag field. Standardize on testing a hash index page's type by doing (opaque->hasho_flag & LH_PAGE_TYPE) == LH_xxx_PAGE. Various places were taking shortcuts like opaque->hasho_flag & LH_BUCKET_PAGE which while not actually wrong, is still bad practice because it encourages use of opaque->hasho_flag & LH_UNUSED_PAGE which *is* wrong (LH_UNUSED_PAGE == 0, so the above is constant false). hash_xlog.c's hash_mask() contained such an incorrect test. This also ensures that we mask out the additional flag bits that hasho_flag has accreted since 9.6. pgstattuple's pgstat_hash_page(), for one, was failing to do that and was thus actively broken. Also fix assorted comments that hadn't been updated to reflect the extended usage of hasho_flag, and fix some macros that were testing just "(hasho_flag & bit)" to use the less dangerous, project-approved form "((hasho_flag & bit) != 0)". Coverity found the bug in hash_mask(); I noted the one in pgstat_hash_page() through code reading.
  • Fix erroneous cross-reference in comment. Seems to have been introduced in commit c219d9b0a. I think there indeed was a "tupbasics.h" in some early drafts of that refactoring, but it didn't survive into the committed version. Amit Kapila
  • Provide a way to control SysV shmem attach address in EXEC_BACKEND builds. In standard non-Windows builds, there's no particular reason to care what address the kernel chooses to map the shared memory segment at. However, when building with EXEC_BACKEND, there's a risk that the chosen address won't be available in all child processes. Linux with ASLR enabled (which it is by default) seems particularly at risk because it puts shmem segments into the same area where it maps shared libraries. We can work around that by specifying a mapping address that's outside the range where shared libraries could get mapped. On x86_64 Linux, 0x7e0000000000 seems to work well. This is only meant for testing/debugging purposes, so it doesn't seem necessary to go as far as providing a GUC (or any user-visible documentation, though we might change that later). Instead, it's just controlled by setting an environment variable PG_SHMEM_ADDR to the desired attach address. Back-patch to all supported branches, since the point here is to remove intermittent buildfarm failures on EXEC_BACKEND animals. Owners of affected animals will need to add a suitable setting of PG_SHMEM_ADDR to their build_env configuration. Discussion:
  • Sync addRangeTableEntryForENR() with its peer functions. addRangeTableEntryForENR had a check for pstate != NULL, which Coverity pointed out was rather useless since it'd already dereferenced pstate before that. More to the point, we'd established policy in commit bc93ac12c that we'd require non-NULL pstate for all addRangeTableEntryFor* functions; this test was evidently copied-and-pasted from some older version of one of those functions. Make it look more like the others. In passing, make an elog message look more like the rest of the code, too. Michael Paquier

Peter Eisentraut pushed:

Robert Haas pushed:

Andres Freund pushed:

Andrew Dunstan pushed:

Michael Meskes pushed:

Magnus Hagander pushed:

  • Remove support for bcc and msvc standalone libpq builds. This removes the support for building just libpq using Borland C++ or Visual C++. This has not worked properly for years, and given the number of complaints it's clearly not worth the maintenance burden. Building libpq using the standard MSVC build system is of course still supported, along with mingw.
  • Remove symbol WIN32_ONLY_COMPILER. This used to mean "Visual C++ except in those parts where Borland C++ was supported where it meant one of those". Now that we don't support Borland C++ anymore, simplify by using _MSC_VER which is the normal way to detect Visual C++.
  • Fix reversed check of return value from sync. While at it also update the comments in walmethods.h to make it less likely for mistakes like this to appear in the future (thanks to Tom for improvements to the comments). And finally, in passing change the return type of walmethod.getlasterror to being const, also per suggestion from Tom.

Bruce Momjian pushed:

Fujii Masao pushed:

Simon Riggs pushed:

Álvaro Herrera pushed:

Correctifs en attente

Michaël Paquier sent in another revision of a patch to use base64-based encoding for stored and server keys in SCRAM verifiers, move the routine to build SCRAM verifier into src/common/, refactor frontend-side random number generation, extend PQencryptPassword with a hashing method, and extend psql's \password and createuser to handle SCRAM.

Michaël Paquier sent in a patch to change pg_hba.conf to refer to SASL, the thing being implemented, instead of SCRAM, one implementation of it.

Ildar Musin sent in a patch to factor out some repetitive code in RI triggers.

Peter Eisentraut sent in two revisions of a patch to fix some warnings generated by gcc7.

David Rowley sent in a patch to allow extended stats on foreign and partitioned tables.

Peter Eisentraut and Bruce Momjian traded patches to fix an issue around HTML rendering in the new documentation toolchain.

Jeff Davis sent in two more revisions of a patch to implement range merge joins.

Alexander Kuzmenkov sent in a patch to implement an index-only count(*) for indexes supporting bitmap scans.

Craig Ringer and Mithun Cy traded patches to make TAP tests run faster.

Kyotaro HORIGUCHI sent in two more revisions of a patch to fix wal_level_minimal.

Amit Langote sent in three more revisions of a patch to fix how pg_dump emits partition attributes.

Masahiko Sawada sent in another revision of a patch to make pg_stat_replication.sync priority report NULL if in quorum-based sync replication, and a different one to change the comments and documentation.

Takayuki Tsunakawa sent in another revision of a patch to make huge pages work on Windows.

Craig Ringer sent in a patch to fix undefined var warnings in PostgresNode with timeout.

Dmitry Ivanov sent in a patch to fix an issue with the custom scan API.

Ashutosh Bapat sent in three more revisions of a patch to fix reporting of violation in ExecConstraints.

Thomas Munro sent in another revision of a patch to implement [[Parallel] Shared] Hash.

Yorick Peterse sent in another revision of a patch to document hot standby config order mentioning all standby servers.

Michaël Paquier sent in another revision of a patch to reimplement pg_upgrade's test as a TAP test.

Pierre Ducroquet sent in a patch to check the results of atoi in pg_basebackup argument parsing.

Amit Langote sent in a patch to ensure that duplicate addition to publication is a no-op.

Petr Jelínek sent in another revision of a patch to set range table for CopyFrom in tablesync.

Petr Jelínek sent in another revision of a patch to reserve global xmin for create slot snasphot export, ensure that on-disk snapshots are not used for snapshot export in logical decoding, prevent snapshot builder xmin from going backwards, fix xl_running_xacts usage in snapshot builder, and skip unnecessary snapshot builds.

Amit Langote sent in a patch to fix a typo in partition.c.

Jeff Janes and Pavan Deolasee traded patches to fix an issue that manifested as PANIC in pg_commit_ts slru after crashes.

Masahiko Sawada sent in a patch to fix a comment typo in xlogutils.c.

Jeff Janes sent in a patch to fix an issue that manifested as failed recovery with the new faster 2PC code.