Correctifs appliqués

Tom Lane pushed:

  • Do some copy-editing on the docs for row-level security. Clarifications, markup improvements, corrections of misleading or outright wrong statements.
  • Fix bogus lock release in RemovePolicyById and RemoveRoleFromObjectPolicy. Can't release the AccessExclusiveLock on the target table until commit. Otherwise there is a race condition whereby other backends might service our cache invalidation signals before they can actually see the updated catalog rows. Just to add insult to injury, RemovePolicyById was closing the rel (with incorrect lock drop) and then passing the now-dangling rel pointer to CacheInvalidateRelcache. Probably the reason this doesn't fall over on CLOBBER_CACHE buildfarm members is that some outer level of the DROP logic is still holding the rel open ... but it'd have bit us on the arse eventually, no doubt.
  • Fix regrole and regnamespace types to honor quoting like other reg* types. Aside from any consistency arguments, this is logically necessary because the I/O functions for these types also handle numeric OID values. Without a quoting rule it is impossible to distinguish numeric OIDs from role or namespace names that happen to contain only digits. Also change the to_regrole and to_regnamespace functions to dequote their arguments. While not logically essential, this seems like a good idea since the other to_reg* functions do it. Anyone who really wants raw lookup of an uninterpreted name can fall back on the time-honored solution of (SELECT oid FROM pg_namespace WHERE nspname = whatever). Report and patch by Jim Nasby, reviewed by Michael Paquier
  • Fix regrole and regnamespace output functions to do quoting, too. We discussed this but somehow failed to implement it...
  • Adjust behavior of row_security GUC to match the docs. Some time back we agreed that row_security=off should not be a way to bypass RLS entirely, but only a way to get an error if it was being applied. However, the code failed to act that way for table owners. Per discussion, this is a must-fix bug for 9.5.0. Adjust the logic in rls.c to behave as expected; also, modify the error message to be more consistent with the new interpretation. The regression tests need minor corrections as well. Also update the comments about row_security in ddl.sgml to be correct. (The official description of the GUC in config.sgml is already correct.) I failed to resist the temptation to do some other very minor cleanup as well, such as getting rid of a duplicate extern declaration.
  • Docs: provide a concrete discussion and example for RLS race conditions. Commit 43cd468cf01007f3 added some wording to create_policy.sgml purporting to warn users against a race condition of the sort that had been noted some time ago by Peter Geoghegan. However, that warning was far too vague to be useful (or at least, I completely failed to grasp what it was on about). Since the problem case occurs with a security design pattern that lots of people are likely to try to use, we need to be as clear as possible about it. Provide a concrete example in the main-line docs in place of the original warning.
  • In psql's tab completion, change most TailMatches patterns to Matches. In the refactoring in commit d37b816dc9e8f976c8913296781e08cbd45c5af1, we mostly kept to the original design whereby only the last few words on the line were matched to identify a completable pattern. However, after commit d854118c8df8c413d069f7e88bb01b9e18e4c8ed, there's really no reason to do it like that: where it's sensible, we can use patterns that expect to match the entire input line. And mostly, it's sensible. Matching the entire line greatly reduces the odds of a false match that leads to offering irrelevant completions. Moreover (though I've not tried to measure this), it should make tab completion faster since many of the patterns will be discarded after a single integer comparison that finds that the wrong number of words appear on the line. There are certain identifiable places where we still need to use TailMatches because the statement in question is allowed to appear embedded in a larger statement. These are just a small minority of the existing patterns, though, so the benefit of switching where possible is large. It's possible that this patch has removed some within-line matching behaviors that are in fact desirable, but we can put those back when we get complaints. Most of the removed behaviors are certainly silly. Michael Paquier, with some further adjustments by me
  • Convert psql's tab completion for backslash commands to the new style. This requires adding some more infrastructure to handle both case-sensitive and case-insensitive matching, as well as the ability to match a prefix of a previous word. So it ends up being about a wash line-count-wise, but it's just as big a readability win here as in the SQL tab completion rules. Michael Paquier, some adjustments by me
  • Add to_regnamespace() and to_regrole() to the documentation. Commits cb9fa802b32b222b and 0c90f6769de6a60f added these functions, but did not bother with documentation.
  • Make the to_reg*() functions accept text not cstring. Using cstring as the input type was a poor decision, because that's not really a full-fledged type. In particular, it lacks implicit coercions from text or varchar, meaning that usages like to_regproc('foo'||'bar') wouldn't work; basically the only case that did work without explicit casting was a simple literal constant argument. The lack of field complaints about this suggests that hardly anyone is using these functions, so hopefully fixing it won't cause much of a compatibility problem. They've only been there since 9.4, anyway. Petr Korobeinikov
  • In opr_sanity regression test, check for unexpected uses of cstring. In light of commit ea0d494dae0d3d6f, it seems like a good idea to add a regression test that will complain about random functions taking or returning cstring. Only I/O support functions and encoding conversion functions should be declared that way. While at it, add some checks that encoding conversion functions are declared properly. Since pg_conversion isn't populated manually, it's not quite as necessary to check its contents as it is for catalogs like pg_proc; but one thing we definitely have not tested in the past is whether the identified conproc for a conversion actually does that conversion vs. some other one.
  • Sort $(wildcard) output where needed for reproducible build output. The order of inclusion of .o files makes a difference in linker output; not a functional difference, but still a bitwise difference, which annoys some packagers who would like reproducible builds. Report and patch by Christoph Berg
  • Remove some ancient and unmaintained encoding-conversion test cruft. In commit 921191912c48a68d I claimed that we weren't testing encoding conversion functions, but further poking around reveals that we did have an equivalent though hard-wired set of tests in conversion.sql. AFAICS there is no advantage to doing it like that as compared to letting the catalog contents drive the test, so let the opr_sanity addition stand and remove the now-redundant tests in conversion.sql. Also, remove some infrastructure in src/backend/utils/mb/conversion_procs for building conversion.sql's list of tests. That was unmaintained, and had not corresponded to the actual contents of conversion.sql since 2007 or perhaps even further back.
  • Comment typo fix. Per Amit Langote.
  • In initdb's post-bootstrap phase, drop temp tables explicitly. Although these temp tables will get removed from template1 at the end of the standalone-backend run, that's too late to keep them from getting copied into the template0 and postgres databases, now that we use only a single backend run for the whole sequence. While no real harm is done by the extra copies (since they'd be deleted on first use of the temp schema), it's still unsightly, and it would mean some wasted cycles for every database creation for the life of the installation. Oversight in commit c4a8812cf64b1426. Noticed by Amit Langote.
  • Provide more detail in postmaster log for password authentication failures. We tell people to examine the postmaster log if they're unsure why they are getting auth failures, but actually only a few relatively-uncommon failure cases were given their own log detail messages in commit 64e43c59b817a78d. Expand on that so that every failure case detected within md5_crypt_verify gets a specific log detail message. This should cover pretty much every ordinary password auth failure cause. So far I've not noticed user demand for a similar level of auth detail for the other auth methods, but sooner or later somebody might want to work on them. This is not that patch, though.
  • Remove vestigial CHECK_FOR_INTERRUPTS call. Commit e710b65c inserted code in md5_crypt_verify to disable and later re-enable interrupts, with a CHECK_FOR_INTERRUPTS call as part of the second step, to process any interrupts that had been held off. Commit 6647248e removed the interrupt disable/re-enable code, but left behind the CHECK_FOR_INTERRUPTS, even though this is now an entirely random, pointless place for one. md5_crypt_verify doesn't run long enough to need such a check, and if it did, this would still be the wrong place to put one.
  • Use plain mkdir() not pg_mkdir_p() to create subdirectories of PGDATA. When we're creating subdirectories of PGDATA during initdb, we know darn well that the parent directory exists (or should exist) and that the new subdirectory doesn't (or shouldn't). There is therefore no need to use anything more complicated than mkdir(). Using pg_mkdir_p() just opens us up to unexpected failure modes, such as the one exhibited in bug #13853 from Nuri Boardman. It's not very clear why pg_mkdir_p() went wrong there, but it is clear that we didn't need to be trying to create parent directories in the first place. We're not even saving any code, as proven by the fact that this patch nets out at minus five lines. Since this is a response to a field bug report, back-patch to all branches.
  • Fix unobvious interaction between -X switch and subdirectory creation. Turns out the only reason initdb -X worked is that pg_mkdir_p won't whine if you point it at something that's a symlink to a directory. Otherwise, the attempt to create pg_xlog/ just like all the other subdirectories would have failed. Let's be a little more explicit about what's happening. Oversight in my patch for bug #13853 (mea culpa for not testing -X ...)
  • Delay creation of subplan tlist until after create_plan(). Once upon a time it was necessary for grouping_planner() to determine the tlist it wanted from the scan/join plan subtree before it called query_planner(), because query_planner() would actually make a Plan using that. But we refactored things a long time ago to delay construction of the Plan tree till later, so there's no need to build that tlist until (and indeed unless) we're ready to plaster it onto the Plan. The only thing query_planner() cares about is what Vars are going to be needed for the tlist, and it can perfectly well get that by looking at the real tlist rather than some masticated version. Well, actually, there is one minor glitch in that argument, which is that make_subplanTargetList also adds Vars appearing only in HAVING to the tlist it produces. So now we have to account for HAVING explicitly in build_base_rel_tlists. But that just adds a few lines of code, and I doubt it moves the needle much on processing time; we might be doing pull_var_clause() twice on the havingQual, but before we had it scanning dummy tlist entries instead. This is a very small down payment on rationalizing grouping_planner enough so it can be refactored.
  • Marginal cleanup of GROUPING SETS code in grouping_planner(). Improve comments and make it a shade less messy. I think we might want to move all of this somewhere else later, but it needs to be more readable first. In passing, re-pgindent the file, affecting some recently-added comments concerning parallel query planning.
  • Add STRICT to some C functions created by the regression tests. These functions readily crash when passed a NULL input value. The tests themselves do not pass NULL values to them; but when the regression database is used as a basis for fuzz testing, they cause a lot of noise. Also, if someone were to leave a regression database lying about in a production installation, these would create a minor security hazard. Andreas Seltenreich
  • Clean up code for widget_in() and widget_out(). Given syntactically wrong input, widget_in() could call atof() with an indeterminate pointer argument, typically leading to a crash; or if it didn't do that, it might return a NULL pointer, which again would lead to a crash since old-style C functions aren't supposed to do things that way. Fix that by correcting the off-by-one syntax test and throwing a proper error rather than just returning NULL. Also, since widget_in and widget_out have been marked STRICT for a long time, their tests for null inputs are just dead code; remove 'em. In the oldest branches, also improve widget_out to use snprintf not sprintf, just to be sure. In passing, get rid of a long-since-useless sprintf into a local buffer that nothing further is done with, and make some other minor coding style cleanups. In the intended regression-testing usage of these functions, none of this is very significant; but if the regression test database were left around in a production installation, these bugs could amount to a minor security hazard. Piotr Stefaniak, Michael Paquier, and Tom Lane
  • Clean up some lack-of-STRICT issues in the core code, too. A scan for missed proisstrict markings in the core code turned up these functions: brin_summarize_new_values pg_stat_reset_single_table_counters pg_stat_reset_single_function_counters pg_create_logical_replication_slot pg_create_physical_replication_slot pg_drop_replication_slot The first three of these take OID, so a null argument will normally look like a zero to them, resulting in "ERROR: could not open relation with OID 0" for brin_summarize_new_values, and no action for the pg_stat_reset_XXX functions. The other three will dump core on a null argument, though this is mitigated by the fact that they won't do so until after checking that the caller is superuser or has rolreplication privilege. In addition, the pg_logical_slot_get/peek[_binary]_changes family was intentionally marked nonstrict, but failed to make nullness checks on all the arguments; so again a null-pointer-dereference crash is possible but only for superusers and rolreplication users. Add the missing ARGISNULL checks to the latter functions, and mark the former functions as strict in pg_proc. Make that change in the back branches too, even though we can't force initdb there, just so that installations initdb'd in future won't have the issue. Since none of these bugs rise to the level of security issues (and indeed the pg_stat_reset_XXX functions hardly misbehave at all), it seems sufficient to do this. In addition, fix some order-of-operations oddities in the slot_get_changes family, mostly cosmetic, but not the part that moves the function's last few operations into the PG_TRY block. As it stood, there was significant risk for an error to exit without clearing historical information from the system caches. The slot_get_changes bugs go back to 9.4 where that code was introduced. Back-patch appropriate subsets of the pg_proc changes into all active branches, as well.
  • Add some checks on "char"-type columns to type_sanity and opr_sanity. I noticed that the sanity checks in the regression tests omitted to check a couple of "poor man's enum" columns that you'd reasonably expect them to check. There are other "char"-type columns in system catalogs that are not covered by either type_sanity or opr_sanity, e.g. pg_rewrite.ev_type. However, those catalogs are not populated with any manually-created data during bootstrap, so it seems less necessary to check them this way.
  • Remove a useless PG_GETARG_DATUM() call from jsonb_build_array. This loop uselessly fetched the argument after the one it's currently looking at. No real harm is done since we couldn't possibly fetch off the end of memory, but it's confusing to the reader. Also remove a duplicate (and therefore confusing) PG_ARGISNULL check in jsonb_build_object. I happened to notice these things while trolling for missed null-arg checks earlier today. Back-patch to 9.5, not because there is any real bug, but just because 9.5 and HEAD are still in sync in this file and we might as well keep them so. In passing, re-pgindent.

Robert Haas pushed:

Ãlvaro Herrera pushed:

  • Make pg_shseclabel available in early backend startup. While the in-core authentication mechanism doesn't need to access pg_shseclabel at all, it's reasonable to think that an authentication hook will want to look at the label for the role logging in, or for rows in other catalogs used during the authentication phase of startup. Catalog version bumped, because this changes the "is nailed" status for pg_shseclabel. Author: Adam Brightwell
  • Make pg_receivexlog silent with 9.3 and older servers. A pointless and confusing error message is shown to the user when attempting to identify a 9.3 or older remote server with a 9.5/9.6 pg_receivexlog, because the return signature of IDENTIFY_SYSTEM was changed in 9.4. There's no good reason for the warning message, so shuffle code around to keep it quiet. (pg_recvlogical is also affected by this commit, but since it obviously cannot work with 9.3 that doesn't actually matter much.) Backpatch to 9.5. Reported by Marco Nenciarini, who also wrote the initial patch. Further tweaked by Robert Haas and Fujii Masao; reviewed by Michael Paquier and Craig Ringer.
  • Add scale(numeric) Author: Marko Tiikkaja
  • Windows: Make pg_ctl reliably detect service status. pg_ctl is using isatty() to verify whether the process is running in a terminal, and if not it sends its output to Windows' Event Log ... which does the wrong thing when the output has been redirected to a pipe, as reported in bug #13592. To fix, make pg_ctl use the code we already have to detect service-ness: in the master branch, move src/backend/port/win32/security.c to src/port (with suitable tweaks so that it runs properly in backend and frontend environments); pg_ctl already has access to pgport so it Just Works. In older branches, that's likely to cause trouble, so instead duplicate the required code in pg_ctl.c. Author: Michael Paquier Bug report and diagnosis: Egon Kocjan Backpatch: all supported branches
  • pgstat: add WAL receiver status view & SRF. This new view provides insight into the state of a running WAL receiver in a HOT standby node. The information returned includes the PID of the WAL receiver process, its status (stopped, starting, streaming, etc), start LSN and TLI, last received LSN and TLI, timestamp of last message send and receipt, latest end-of-WAL LSN and time, and the name of the slot (if any). Access to the detailed data is only granted to superusers; others only get the PID. Author: Michael Paquier Reviewer: Haribabu Kommi
  • Add win32security to LIBOBJS. This seems to fix Mingw's compile that was broken in a967613911f7e, as evidenced by buildfarm.
  • Fix order of arguments to va_start()
  • Blind attempt at a Cygwin fix. Further portability fix for a967613911f7. Mingw- and MSVC-based builds appear to be working fine, but Cygwin needs an extra tweak whereby the new win32security.c file is explicitely added to the list of files to build in pgport, per Cygwin members brolga and lorikeet. Author: Michael Paquier
  • Revert "Blind attempt at a Cygwin fix." This reverts commit e9282e953205a2f3125fc8d1052bc01cb77cd2a3, which blew up in a pretty spectacular way. Re-introduce the original code while we search for a real fix.

Tatsuo Ishii pushed:

Magnus Hagander pushed:

Simon Riggs pushed:

  • Avoid pin scan for replay of XLOG_BTREE_VACUUM. Replay of XLOG_BTREE_VACUUM during Hot Standby was previously thought to require complex interlocking that matched the requirements on the master. This required an O(N) operation that became a significant problem with large indexes, causing replication delays of seconds or in some cases minutes while the XLOG_BTREE_VACUUM was replayed. This commit skips the “pin scan†that was previously required, by observing in detail when and how it is safe to do so, with full documentation. The pin scan is skipped only in replay; the VACUUM code path on master is not touched here. The current commit still performs the pin scan for toast indexes, though this can also be avoided if we recheck scans on toast indexes. Later patch will address this. No tests included. Manual tests using an additional patch to view WAL records and their timing have shown the change in WAL records and their handling has successfully reduced replication delay.
  • Revoke change to rmgr desc of btree vacuum. Per discussion with Andres Freund

Correctifs rejetés (à ce jour)

No one was disappointed this week :-)

Correctifs en attente

Ian Lawrence Barwick sent in a patch to update the description of vacuumdb to fit current realities.

Haribabu Kommi sent in four more revisions of a patch to implement multi-tenancy with RLS.

Pavel Stěhule sent in another revision of a patch to implement num_notnulls().

Pavel Stěhule sent in three more revisions of a patch to implement pg_size_bytes().

Michael Paquier sent in another revision of a patch to ensure that dynloader.h be present in Windows installations.

Amul Sul sent in a patch to ensure that columns in tables made inheritance tables by ALTER TABLE ... INHERIT are actually dropped when they're dropped in the parents.

Vitaly Burovoy sent in another revision of a patch to allow non-crazy extraction of fields from 'infinity'::TIMESTAMP[TZ].

Alexander Shulgin sent in a patch to fix an inconsistency in error handling for the START_REPLICATION command.

Petr Korobeinikov sent in a patch to add schema-qualified relnames in constraint error messages.

Alexander Shulgin sent in a patch to add an \errverbose option to psql to include schema-qualified names in error messages.

Amit Kapila sent in two more revisions of a patch to refactor LWLock tranches.

Andreas Karlsson sent in another revision of a patch to add COPY (statement) tab completion to psql.

Amit Kapila sent in another revision of a patch to rename pgproc variables.

Andreas Karlsson sent in another revision of a patch to improve tab completion in psql for FDW DDL.

Kyotaro HORIGUCHI sent in another revision of a patch to prepare for sharing psqlscan with pgbench, change the access method to shell variables, detach common.c from psqlscan, ensure that pgbench uses a common frontend SQL parser, and change the way to hold command list.

Ashutosh Bapat sent in another revision of a patch to enable getting sorted data from foreign server for merge joins.

Ashutosh Bapat sent in a patch to remove duplicate parameter deparser code from postgres_fdw.

Craig Ringer sent in another revision of a patch to implement pg_logical_output.

Marisa Emerson sent in two revisions of a patch to add support for BSD authentication, used on OpenBSD.

Etsuro Fujita sent in a patch to fix an issue with updating foreign tables.

Peter Geoghegan sent in another revision of a patch to generalize SortSupport for text.

Konstantin Knizhnik sent in two revisions of a patch to optimize LIMIT clauses.

Craig Ringer sent in another revision of a patch to implement pglogical.

David Rowley sent in another revision of a patch to implement combining aggregates.

Ashutosh Bapat sent in a patch to fix some infelicities in FDW join pushdown and scanclauses.

Ãlvaro Herrera sent in another revision of a patch to allow cataloging NOT NULL constraints.

Tomas Vondra sent in another revision of a patch to extend the HyperLogLog API.

Stas Kelvich and Simon Riggs traded patches to speed up two-phase transactions.

Peter Geoghegan sent in a patch to fix some misspellings in nodeGather.c and controldata.c.

Michael Paquier sent in another revision of a patch to fix some issues with Cygwin.

Peter Eisentraut sent in a patch to fix some odd bugs in psql's tab completion of CREATE INDEX.

Christian Ullrich sent in another revision of a patch to fix an SSPI auth bug.

Christian Ullrich sent in a patch to close a handle leak in SSPI auth.