1. 05 Jan, 2016 1 commit
    • Tom Lane's avatar
      In psql's tab completion, change most TailMatches patterns to Matches. · 9b181b03
      Tom Lane authored
      In the refactoring in commit d37b816d,
      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 d854118c, 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
      9b181b03
  2. 04 Jan, 2016 7 commits
    • Tom Lane's avatar
      Docs: provide a concrete discussion and example for RLS race conditions. · 7debf360
      Tom Lane authored
      Commit 43cd468c 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.
      7debf360
    • Tom Lane's avatar
      Adjust behavior of row_security GUC to match the docs. · 5d354382
      Tom Lane authored
      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.
      5d354382
    • Robert Haas's avatar
      Fix typo in comment. · 8978eb03
      Robert Haas authored
      Masahiko Sawada
      8978eb03
    • Tom Lane's avatar
      Fix regrole and regnamespace output functions to do quoting, too. · b0cadc08
      Tom Lane authored
      We discussed this but somehow failed to implement it...
      b0cadc08
    • Tom Lane's avatar
      Fix regrole and regnamespace types to honor quoting like other reg* types. · fb1227af
      Tom Lane authored
      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
      fb1227af
    • Tom Lane's avatar
      Fix bogus lock release in RemovePolicyById and RemoveRoleFromObjectPolicy. · f47b602d
      Tom Lane authored
      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.
      f47b602d
    • Tom Lane's avatar
      Do some copy-editing on the docs for row-level security. · c1611db0
      Tom Lane authored
      Clarifications, markup improvements, corrections of misleading or
      outright wrong statements.
      c1611db0
  3. 03 Jan, 2016 5 commits
    • Tom Lane's avatar
      Guard against null arguments in binary_upgrade_create_empty_extension(). · 939d10cd
      Tom Lane authored
      The CHECK_IS_BINARY_UPGRADE macro is not sufficient security protection
      if we're going to dereference pass-by-reference arguments before it.
      
      But in any case we really need to explicitly check PG_ARGISNULL for all
      the arguments of a non-strict function, not only the ones we expect null
      values for.
      
      Oversight in commits 30982be4 and
      f92fc4c9.  Found by Andreas Seltenreich.
      (The other usages in pg_upgrade_support.c seem safe.)
      939d10cd
    • Tom Lane's avatar
      Do some copy-editing on the docs for replication origins. · c6aeba35
      Tom Lane authored
      Minor grammar and markup improvements.
      c6aeba35
    • Tom Lane's avatar
      02798919
    • Tom Lane's avatar
      Fix treatment of *lpNumberOfBytesRecvd == 0: that's a completion condition. · 90e61df8
      Tom Lane authored
      pgwin32_recv() has treated a non-error return of zero bytes from WSARecv()
      as being a reason to block ever since the current implementation was
      introduced in commit a4c40f14.  However, so far as one can tell
      from Microsoft's documentation, that is just wrong: what it means is
      graceful connection closure (in stream protocols) or receipt of a
      zero-length message (in message protocols), and neither case should result
      in blocking here.  The only reason the code worked at all was that control
      then fell into the retry loop, which did *not* treat zero bytes specially,
      so we'd get out after only wasting some cycles.  But as of 9.5 we do not
      normally reach the retry loop and so the bug is exposed, as reported by
      Shay Rojansky and diagnosed by Andres Freund.
      
      Remove the unnecessary test on the byte count, and rearrange the code
      in the retry loop so that it looks identical to the initial sequence.
      
      Back-patch to 9.5.  The code is wrong all the way back, AFAICS, but
      since it's relatively harmless in earlier branches we'll leave it alone.
      90e61df8
    • Tom Lane's avatar
      Teach pg_dump to quote reloption values safely. · b416c0bb
      Tom Lane authored
      Commit c7e27bec fixed this on the backend side, but we neglected
      the fact that several code paths in pg_dump were printing reloptions
      values that had not gotten massaged by ruleutils.  Apply essentially the
      same quoting logic in those places, too.
      b416c0bb
  4. 02 Jan, 2016 7 commits
    • Tom Lane's avatar
      Fix overly-strict assertions in spgtextproc.c. · 7157fe80
      Tom Lane authored
      spg_text_inner_consistent is capable of reconstructing an empty string
      to pass down to the next index level; this happens if we have an empty
      string coming in, no prefix, and a dummy node label.  (In practice, what
      is needed to trigger that is insertion of a whole bunch of empty-string
      values.)  Then, we will arrive at the next level with in->level == 0
      and a non-NULL (but zero length) in->reconstructedValue, which is valid
      but the Assert tests weren't expecting it.
      
      Per report from Andreas Seltenreich.  This has no impact in non-Assert
      builds, so should not be a problem in production, but back-patch to
      all affected branches anyway.
      
      In passing, remove a couple of useless variable initializations and
      shorten the code by not duplicating DatumGetPointer() calls.
      7157fe80
    • Tom Lane's avatar
      Adjust back-branch release note description of commits a2a718b22 et al. · df35af2c
      Tom Lane authored
      As pointed out by Michael Paquier, recovery_min_apply_delay didn't exist
      in 9.0-9.3, making the release note text not very useful.  Instead make it
      talk about recovery_target_xid, which did exist then.
      
      9.0 is already out of support, but we can fix the text in the newer
      branches' copies of its release notes.
      df35af2c
    • Tom Lane's avatar
      Make copyright.pl cope with nonstandard case choices in copyright notices. · de7c8dbe
      Tom Lane authored
      The need for this is shown by the files it missed in Bruce's recent run.
      I fixed it so that it will actually adjust the case when needed.
      
      In passing, also make it skip .po files, since those will just get
      overwritten anyway from the translation repository.
      de7c8dbe
    • Tom Lane's avatar
      Update copyright for 2016 · 48c9f288
      Tom Lane authored
      On closer inspection, the reason copyright.pl was missing files is
      that it is looking for 'Copyright (c)' and they had 'Copyright (C)'.
      Fix that, and update a couple more that grepping for that revealed.
      48c9f288
    • Tom Lane's avatar
      Update copyright for 2016 · ad08bf5c
      Tom Lane authored
      Manually fix some copyright lines missed by the automated script.
      ad08bf5c
    • Bruce Momjian's avatar
      Update copyright for 2016 · ee943004
      Bruce Momjian authored
      Backpatch certain files through 9.1
      ee943004
    • Noah Misch's avatar
      Cover heap_page_prune_opt()'s cleanup lock tactic in README. · dfcd9cb3
      Noah Misch authored
      Jeff Janes, reviewed by Jim Nasby.
      dfcd9cb3
  5. 01 Jan, 2016 5 commits
    • Tom Lane's avatar
      Teach flatten_reloptions() to quote option values safely. · c7e27bec
      Tom Lane authored
      flatten_reloptions() supposed that it didn't really need to do anything
      beyond inserting commas between reloption array elements.  However, in
      principle the value of a reloption could be nearly anything, since the
      grammar allows a quoted string there.  Any restrictions on it would come
      from validity checking appropriate to the particular option, if any.
      
      A reloption value that isn't a simple identifier or number could thus lead
      to dump/reload failures due to syntax errors in CREATE statements issued
      by pg_dump.  We've gotten away with not worrying about this so far with
      the core-supported reloptions, but extensions might allow reloption values
      that cause trouble, as in bug #13840 from Kouhei Sutou.
      
      To fix, split the reloption array elements explicitly, and then convert
      any value that doesn't look like a safe identifier to a string literal.
      (The details of the quoting rule could be debated, but this way is safe
      and requires little code.)  While we're at it, also quote reloption names
      if they're not safe identifiers; that may not be a likely problem in the
      field, but we might as well try to be bulletproof here.
      
      It's been like this for a long time, so back-patch to all supported
      branches.
      
      Kouhei Sutou, adjusted some by me
      c7e27bec
    • Tom Lane's avatar
      Add some more defenses against silly estimates to gincostestimate(). · 3c93a60f
      Tom Lane authored
      A report from Andy Colson showed that gincostestimate() was not being
      nearly paranoid enough about whether to believe the statistics it finds in
      the index metapage.  The problem is that the metapage stats (other than the
      pending-pages count) are only updated by VACUUM, and in the worst case
      could still reflect the index's original empty state even when it has grown
      to many entries.  We attempted to deal with that by scaling up the stats to
      match the current index size, but if nEntries is zero then scaling it up
      still gives zero.  Moreover, the proportion of pages that are entry pages
      vs. data pages vs. pending pages is unlikely to be estimated very well by
      scaling if the index is now orders of magnitude larger than before.
      
      We can improve matters by expanding the use of the rule-of-thumb estimates
      I introduced in commit 7fb008c5: if the index has grown by more
      than a cutoff amount (here set at 4X growth) since VACUUM, then use the
      rule-of-thumb numbers instead of scaling.  This might not be exactly right
      but it seems much less likely to produce insane estimates.
      
      I also improved both the scaling estimate and the rule-of-thumb estimate
      to account for numPendingPages, since it's reasonable to expect that that
      is accurate in any case, and certainly pages that are in the pending list
      are not either entry or data pages.
      
      As a somewhat separate issue, adjust the estimation equations that are
      concerned with extra fetches for partial-match searches.  These equations
      suppose that a fraction partialEntries / numEntries of the entry and data
      pages will be visited as a consequence of a partial-match search.  Now,
      it's physically impossible for that fraction to exceed one, but our
      estimate of partialEntries is mostly bunk, and our estimate of numEntries
      isn't exactly gospel either, so we could arrive at a silly value.  In the
      example presented by Andy we were coming out with a value of 100, leading
      to insane cost estimates.  Clamp the fraction to one to avoid that.
      
      Like the previous patch, back-patch to all supported branches; this
      problem can be demonstrated in one form or another in all of them.
      3c93a60f
    • Noah Misch's avatar
      Fix comments about WAL rule "write xlog before data" versus pg_multixact. · 3cd1ba14
      Noah Misch authored
      Recovery does not achieve its goal of zeroing all pg_multixact entries
      whose accompanying WAL records never reached disk.  Remove that claim
      and justify its expendability.  Detail the need for TrimMultiXact(),
      which has little in common with the TrimCLOG() rationale.  Merge two
      tightly-related comments.  Stop presenting pg_multixact as specific to
      heap_lock_tuple(); PostgreSQL 9.3 extended its use to heap_update().
      
      Noticed while investigating a report from Andres Freund.
      3cd1ba14
    • Peter Eisentraut's avatar
      doc: Remove redundant duplicate URLs from ulink elements · 253de19b
      Peter Eisentraut authored
      Empty ulink elements default to displaying the URL, so there is no need
      to specify the URL again.  This was already done for most occurrences,
      but some cases didn't follow this convention.
      253de19b
    • Peter Eisentraut's avatar
  6. 31 Dec, 2015 2 commits
    • Tom Lane's avatar
      Add a comment noting that FDWs don't have to implement EXCEPT or LIMIT TO. · 5f36096b
      Tom Lane authored
      postgresImportForeignSchema pays attention to IMPORT's EXCEPT and LIMIT TO
      options, but only as an efficiency hack, not for correctness' sake.  The
      FDW documentation does explain that, but someone using postgres_fdw.c
      as a coding guide might not remember it, so let's add a comment here.
      Per question from Regina Obe.
      5f36096b
    • Tom Lane's avatar
      Fix ALTER OPERATOR to update dependencies properly. · 0dab5ef3
      Tom Lane authored
      Fix an oversight in commit 321eed5f: replacing an operator's
      selectivity functions needs to result in a corresponding update in
      pg_depend.  We have a function that can handle that, but it was not
      called by AlterOperator().
      
      To fix this without enlarging pg_operator.h's #include list beyond
      what clients can safely include, split off the function definitions
      into a new file pg_operator_fn.h, similarly to what we've done for
      some other catalog header files.  It's not entirely clear whether
      any client-side code needs to include pg_operator.h, but it seems
      prudent to assume that there is some such code somewhere.
      0dab5ef3
  7. 30 Dec, 2015 3 commits
    • Tom Lane's avatar
      Dept of second thoughts: the !scan_all exit mustn't increase scanned_pages. · e5d06f2b
      Tom Lane authored
      In the extreme edge case where contended pages are the only ones that
      escape being scanned, the previous commit would have allowed us to think
      that relfrozenxid could be advanced, which is exactly wrong.
      e5d06f2b
    • Tom Lane's avatar
      Avoid useless truncation attempts during VACUUM. · e8429082
      Tom Lane authored
      VACUUM can skip heap pages altogether when there's a run of consecutive
      pages that are all-visible according to the visibility map.  This causes it
      to not update its nonempty_pages count, just as if those pages were empty,
      which means that at the end we will think they are candidates for deletion.
      Thus, we may take the table's AccessExclusive lock only to find that no
      pages are really truncatable.  This usually causes no real problems on a
      master server, thanks to the lock being acquired only conditionally; but on
      hot-standby servers, the same lock must be acquired unconditionally which
      can result in unnecessary query cancellations.
      
      To improve matters, force examination of the table's last page whenever
      we reach there with a nonempty_pages count that would allow a truncation
      attempt.  If it's not empty, we'll advance nonempty_pages and thereby
      prevent the truncation attempt.
      
      If we are unable to acquire cleanup lock on that page, there's no need to
      force it, unless we're doing an anti-wraparound vacuum.  We can just check
      for tuples with a shared buffer lock and then give up.  (When we are doing
      an anti-wraparound vacuum, and decide it's okay to skip the page because it
      contains no freezable tuples, this patch still improves matters because
      nonempty_pages is properly updated, which it was not before.)
      
      Since only the last page is special-cased in this way, we might attempt a
      truncation that will release many fewer pages than the normal heuristic
      would suggest; at worst, only one page would be truncated.  But that seems
      all right, because the situation won't repeat during the next vacuum.
      The real problem with the old logic is that the useless truncation attempt
      happens every time we vacuum, so long as the state of the last few dozen
      pages doesn't change.
      
      This is a longstanding deficiency, but since the consequences aren't very
      severe in most scenarios, I'm not going to risk a back-patch.
      
      Jeff Janes and Tom Lane
      e8429082
    • Tom Lane's avatar
      Minor hacking on contrib/cube documentation. · e5e5267a
      Tom Lane authored
      Improve markup, particularly of the table of functions; add or improve
      examples for some of the functions; wordsmith some of the function
      descriptions.
      e5e5267a
  8. 29 Dec, 2015 2 commits
    • Tom Lane's avatar
      Add some comments about division of labor between rewriter and planner. · efe4c9d7
      Tom Lane authored
      The rationale for the way targetlist processing is done wasn't clearly
      stated anywhere, and I for one had forgotten some of the details.  Having
      just painfully re-learned them, add some breadcrumbs for the next person.
      efe4c9d7
    • Tom Lane's avatar
      Put back one copyObject() in rewriteTargetView(). · fd195257
      Tom Lane authored
      Commit 6f8cb1e2 tried to centralize rewriteTargetView's copying
      of a target view's Query struct.  However, it ignored the fact that the
      jointree->quals field was used twice.  This only accidentally failed to
      fail immediately because the same ChangeVarNodes mutation is applied in
      both cases, so that we end up with logically identical expression trees
      for both uses (and, as the code stands, the second ChangeVarNodes call
      actually does nothing).  However, we end up linking *physically*
      identical expression trees into both an RTE's securityQuals list and
      the WithCheckOption list.  That's pretty dangerous, mainly because
      prepsecurity.c is utterly cavalier about further munging such structures
      without copying them first.
      
      There may be no live bug in HEAD as a consequence of the fact that we apply
      preprocess_expression in between here and prepsecurity.c, and that will
      make a copy of the tree anyway.  Or it may just be that the regression
      tests happen to not trip over it.  (I noticed this only because things
      fell over pretty badly when I tried to relocate the planner's call of
      expand_security_quals to before expression preprocessing.)  In any case
      it's very fragile because if anyone tried to make the securityQuals and
      WithCheckOption trees diverge before we reach preprocess_expression, it
      would not work.  The fact that the current code will preprocess
      securityQuals and WithCheckOptions lists at completely different times in
      different query levels does nothing to increase my trust that that can't
      happen.
      
      In view of the fact that 9.5.0 is almost upon us and the aforesaid commit
      has seen exactly zero field testing, the prudent course is to make an extra
      copy of the quals so that the behavior is not different from what has been
      in the field during beta.
      fd195257
  9. 28 Dec, 2015 8 commits
    • Joe Conway's avatar
      Rename (new|old)estCommitTs to (new|old)estCommitTsXid · 241448b2
      Joe Conway authored
      The variables newestCommitTs and oldestCommitTs sound as if they are
      timestamps, but in fact they are the transaction Ids that correspond
      to the newest and oldest timestamps rather than the actual timestamps.
      Rename these variables to reflect that they are actually xids: to wit
      newestCommitTsXid and oldestCommitTsXid respectively. Also modify
      related code in a similar fashion, particularly the user facing output
      emitted by pg_controldata and pg_resetxlog.
      
      Complaint and patch by me, review by Tom Lane and Alvaro Herrera.
      Backpatch to 9.5 where these variables were first introduced.
      241448b2
    • Tom Lane's avatar
      Code and docs review for cube kNN support. · 81ee726d
      Tom Lane authored
      Commit 33bd250f could have done with
      some more review:
      
      Adjust coding so that compilers unfamiliar with elog/ereport don't complain
      about uninitialized values.
      
      Fix misuse of PG_GETARG_INT16 to retrieve arguments declared as "integer"
      at the SQL level.  (This was evidently copied from cube_ll_coord and
      cube_ur_coord, but those were wrong too.)
      
      Fix non-style-guide-conforming error messages.
      
      Fix underparenthesized if statements, which pgindent would have made a
      hash of, and remove some unnecessary parens elsewhere.
      
      Run pgindent over new code.
      
      Revise documentation: repeated accretion of more operators without any
      rethinking of the text already there had left things in a bit of a mess.
      Merge all the cube operators into one table and adjust surrounding text
      appropriately.
      
      David Rowley and Tom Lane
      81ee726d
    • Alvaro Herrera's avatar
      Document brin_summarize_new_pages · ac443d10
      Alvaro Herrera authored
      Pointer out by Jeff Janes
      ac443d10
    • Tom Lane's avatar
      Document the exponentiation operator as associating left to right. · 54aaafe9
      Tom Lane authored
      Common mathematical convention is that exponentiation associates right to
      left.  We aren't going to change the parser for this, but we could note
      it in the operator's description.  (It's already noted in the operator
      precedence/associativity table, but users might not look there.)
      Per bug #13829 from Henrik Pauli.
      54aaafe9
    • Tom Lane's avatar
      Fix omission of -X (--no-psqlrc) in some psql invocations. · 870df2b3
      Tom Lane authored
      As of commit d5563d7d, psql -c no longer implies -X, but not all of
      our regression testing scripts had gotten that memo.
      
      To ensure consistency of results across different developers, make
      sure that *all* invocations of psql in all scripts in our tree
      use -X, even where this is not what previously happened.
      
      Michael Paquier and Tom Lane
      870df2b3
    • Alvaro Herrera's avatar
      doc: pg_committs -> pg_commit_ts · 151c4ffe
      Alvaro Herrera authored
      Reported by: Alain Laporte (#13836)
      151c4ffe
    • Tom Lane's avatar
      Update documentation about pseudo-types. · 731dfc7d
      Tom Lane authored
      Tone down an overly strong statement about which pseudo-types PLs are
      likely to allow.  Add "event_trigger" to the list, as well as
      "pg_ddl_command" in 9.5/HEAD.  Back-patch to 9.3 where event_trigger
      was added.
      731dfc7d
    • Alvaro Herrera's avatar
      Fix translation domain in pg_basebackup · fc995bfd
      Alvaro Herrera authored
      For some reason, we've been overlooking the fact that pg_receivexlog
      and pg_recvlogical are using wrong translation domains all along,
      so their output hasn't ever been translated.  The right domain is
      pg_basebackup, not their own executable names.
      
      Noticed by Ioseph Kim, who's been working on the Korean translation.
      
      Backpatch pg_receivexlog to 9.2 and pg_recvlogical to 9.4.
      fc995bfd