1. 28 Apr, 2019 4 commits
    • Tom Lane's avatar
      Do pre-release housekeeping on catalog data, and fix jsonpath send/recv. · c3f67ed6
      Tom Lane authored
      Run renumber_oids.pl to move high-numbered OIDs down, as per pre-beta
      tasks specified by RELEASE_CHANGES.  (The only change is 8394 -> 3428.)
      
      Also run reformat_dat_file.pl while I'm here.
      
      While looking at the reformat diffs, I chanced to notice that type
      jsonpath had typsend and typreceive = '-', which surely is not the
      intention given that jsonpath_send and jsonpath_recv exist.
      Fix that.  It's safe to assume that these functions have never been
      tested :-(.  I didn't try, but somebody should.
      c3f67ed6
    • Noah Misch's avatar
      Use preprocessor conditions compatible with Emacs indent. · 90e7f317
      Noah Misch authored
      Emacs wrongly indented hundreds of subsequent lines.
      90e7f317
    • Tom Lane's avatar
      Clean up minor warnings from buildfarm. · e481d262
      Tom Lane authored
      Be more consistent about use of XXXGetDatum macros in new jsonpath
      code.  This is mostly to avoid having code that looks randomly
      different from everyplace else that's doing the exact same thing.
      
      In pg_regress.c, avoid an unreferenced-function warning from
      compilers that don't understand pg_attribute_unused().  Putting
      the function inside the same #ifdef as its only caller is more
      straightforward coding anyway.
      
      In be-secure-openssl.c, avoid use of pg_attribute_unused() on a label.
      That's pretty creative, but there's no good reason to suppose that
      it's portable, and there's absolutely no need to use goto's here in the
      first place.  (This wasn't actually causing any buildfarm complaints,
      but it's new code in v12 so it has no portability track record.)
      e481d262
    • Michael Paquier's avatar
      Fix more typos and inconsistencies in documentation · ac862376
      Michael Paquier authored
      This fixes a couple of grammar mistakes, typos and inconsistencies in
      the documentation.  Particularly, the configuration parsing allows only
      "kB" to mean kilobyte but there were references in the docs to "KB".
      Some instances of the latter are still in the code comments.  Some
      parameter values were mentioned with "Minus-one", and using directly
      "-1" with proper markups is more helpful to the reader.
      
      Some of these have been pointed out by Justin, and some others are
      things I bumped into.
      
      Author: Justin Pryzby, Michael Paquier
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/20190330224333.GQ5815@telsasoft.com
      ac862376
  2. 27 Apr, 2019 7 commits
    • Tom Lane's avatar
      Avoid postgres_fdw crash for a targetlist entry that's just a Param. · 8cad5adb
      Tom Lane authored
      foreign_grouping_ok() is willing to put fairly arbitrary expressions into
      the targetlist of a remote SELECT that's doing grouping or aggregation on
      the remote side, including expressions that have no foreign component to
      them at all.  This is possibly a bit dubious from an efficiency standpoint;
      but it rises to the level of a crash-causing bug if the expression is just
      a Param or non-foreign Var.  In that case, the expression will necessarily
      also appear in the fdw_exprs list of values we need to send to the remote
      server, and then setrefs.c's set_foreignscan_references will mistakenly
      replace the fdw_exprs entry with a Var referencing the targetlist result.
      
      The root cause of this problem is bad design in commit e7cb7ee1: it put
      logic into set_foreignscan_references that IMV is postgres_fdw-specific,
      and yet this bug shows that it isn't postgres_fdw-specific enough.  The
      transformation being done on fdw_exprs assumes that fdw_exprs is to be
      evaluated with the fdw_scan_tlist as input, which is not how postgres_fdw
      uses it; yet it could be the right thing for some other FDW.  (In the
      bigger picture, setrefs.c has no business assuming this for the other
      expression fields of a ForeignScan either.)
      
      The right fix therefore would be to expand the FDW API so that the
      FDW could inform setrefs.c how it intends to evaluate these various
      expressions.  We can't change that in the back branches though, and we
      also can't just summarily change setrefs.c's behavior there, or we're
      likely to break external FDWs.
      
      As a stopgap, therefore, hack up postgres_fdw so that it won't attempt
      to send targetlist entries that look exactly like the fdw_exprs entries
      they'd produce.  In most cases this actually produces a superior plan,
      IMO, with less data needing to be transmitted and returned; so we probably
      ought to think harder about whether we should ship tlist expressions at
      all when they don't contain any foreign Vars or Aggs.  But that's an
      optimization not a bug fix so I left it for later.  One case where this
      produces an inferior plan is where the expression in question is actually
      a GROUP BY expression: then the restriction prevents us from using remote
      grouping.  It might be possible to work around that (since that would
      reduce to group-by-a-constant on the remote side); but it seems like a
      pretty unlikely corner case, so I'm not sure it's worth expending effort
      solely to improve that.  In any case the right long-term answer is to fix
      the API as sketched above, and then revert this hack.
      
      Per bug #15781 from Sean Johnston.  Back-patch to v10 where the problem
      was introduced.
      
      Discussion: https://postgr.es/m/15781-2601b1002bad087c@postgresql.org
      8cad5adb
    • Joe Conway's avatar
      Add viewBox attribute to storage page layout SVG image · 29046c44
      Joe Conway authored
      Recently added SVG image for storage page layout lacks
      a viewBox attribute which seems necessary to ensure propoer
      rendering. Add it.
      
      Author: Jonathan Katz
      Discussion: https://postgr.es/m/ba31e0e1-4c9b-b309-70e8-8e7ac14fc87e%40postgresql.org
      29046c44
    • Joe Conway's avatar
      Add guidance on making documentation SVG images responsive · 7dc78d8e
      Joe Conway authored
      Recently added guidance on adding SVG images to the documentation
      sources lacks advice on making the images responsive when rendered
      in a variety of media types and viewports. Add some.
      
      Patch by Jonathan Katz with some editorialization by me.
      
      Author: Jonathan Katz
      Discussion: https://postgr.es/m/6358ae6f-7191-a02b-e7b5-68050636ae71@postgresql.org
      7dc78d8e
    • Joe Conway's avatar
      Correct the URL pointing to PL/R · cf3ff97a
      Joe Conway authored
      As pointed out by documentation comment, the URL for PL/R
      needs to be updated to the correct current repository. Back-patch
      to all supported branches.
      cf3ff97a
    • Peter Eisentraut's avatar
      Update config.guess and config.sub · ddcaa596
      Peter Eisentraut authored
      ddcaa596
    • Tom Lane's avatar
      Portability fix for zic.c. · 48f8fa9c
      Tom Lane authored
      Missed an inttypes.h dependency in previous patch.  Per buildfarm.
      48f8fa9c
    • Michael Paquier's avatar
      Mention REINDEX CONCURRENTLY in documentation about index maintenance · a9678784
      Michael Paquier authored
      The documentation includes a section about index maintenance and
      reindexing, mentioning a set of steps based on CREATE INDEX CONCURRENTLY
      and ALTER TABLE (for constraint dependencies) to emulate REINDEX
      CONCURRENTLY.  Now that REINDEX CONCURRENTLY is supported, let's just
      directly mention it instead.
      
      Reported-by: Peter Geoghegan
      Author: Michael Paquier
      Reviewed-by: Peter Eisentraut, Tom Lane
      Discussion: https://postgr.es/m/CAH2-WzmEL168t6w29aKrKXtpq9-apcmp0HC7K-fKt6ZgLXV6Dg@mail.gmail.com
      a9678784
  3. 26 Apr, 2019 7 commits
    • Tom Lane's avatar
      Sync our copy of the timezone library with IANA release tzcode2019a. · acb897b8
      Tom Lane authored
      This corrects a small bug in zic that caused it to output an incorrect
      year-2440 transition in the Africa/Casablanca zone.
      
      More interestingly, zic has grown a "-r" option that limits the range of
      zone transitions that it will put into the output files.  That might be
      useful to people who don't like the weird GMT offsets that tzdb likes
      to use for very old dates.  It appears that for dates before the cutoff
      time specified with -r, zic will use the zone's standard-time offset
      as of the cutoff time.  So for example one might do
      
      	make install ZIC_OPTIONS='-r @-1893456000'
      
      to cause all dates before 1910-01-01 to be treated as though 1910
      standard time prevailed indefinitely far back.  (Don't blame me for
      the unfriendly way of specifying the cutoff time --- it's seconds
      since or before the Unix epoch.  You can use extract(epoch ...)
      to calculate it.)
      
      As usual, back-patch to all supported branches.
      acb897b8
    • Tom Lane's avatar
      Update time zone data files to tzdata release 2019a. · d312de3f
      Tom Lane authored
      DST law changes in Palestine and Metlakatla.
      Historical corrections for Israel.
      
      Etc/UCT is now a backward-compatibility link to Etc/UTC, instead
      of being a separate zone that generates the abbreviation "UCT",
      which nowadays is typically a typo.  Postgres will still accept
      "UCT" as an input zone name, but it won't output it.
      d312de3f
    • Peter Eisentraut's avatar
      Update key words table for version 12 · 686fef22
      Peter Eisentraut authored
      686fef22
    • Tom Lane's avatar
      Apply stopgap fix for bug #15672. · c01eb619
      Tom Lane authored
      Fix DefineIndex so that it doesn't attempt to pass down a to-be-reused
      index relfilenode to a child index creation, and fix TryReuseIndex
      to not think that reuse is sensible for a partitioned index.
      
      In v11, this fixes a problem where ALTER TABLE on a partitioned table
      could assign the same relfilenode to several different child indexes,
      causing very nasty catalog corruption --- in fact, attempting to DROP
      the partitioned table then leads not only to a database crash, but to
      inability to restart because the same crash will recur during WAL replay.
      
      Either of these two changes would be enough to prevent the failure, but
      since neither action could possibly be sane, let's put in both changes
      for future-proofing.
      
      In HEAD, no such bug manifests, but that's just an accidental consequence
      of having changed the pg_class representation of partitioned indexes to
      have relfilenode = 0.  Both of these changes still seem like smart
      future-proofing.
      
      This is only a stop-gap because the code for ALTER TABLE on a partitioned
      table with a no-op type change still leaves a great deal to be desired.
      As the added regression tests show, it gets things wrong for comments on
      child indexes/constraints, and it is regenerating child indexes it doesn't
      have to.  However, fixing those problems will take more work which may not
      get back-patched into v11.  We need a fix for the corruption problem now.
      
      Per bug #15672 from Jianing Yang.
      
      Patch by me, regression test cases based on work by Amit Langote,
      who also did a lot of the investigative work.
      
      Discussion: https://postgr.es/m/15672-b9fa7db32698269f@postgresql.org
      c01eb619
    • Alvaro Herrera's avatar
      pg_dump: store unused attribs as NULL instead of '\0' · 7fcdb5e0
      Alvaro Herrera authored
      Commit f831d4ac changed pg_dump to emit (and pg_restore to
      understand) NULLs for unused members in ArchiveEntry structs, as a side
      effect of some code beautification.  That broke pg_restore of dumps
      generated with older pg_dump, however, so it was reverted in
      19455c9f.  Since the archiver version number has been bumped in
      3b925e90, we can put it back.
      
      Author: Dmitry Dolgov
      Discussion: https://postgr.es/m/CA+q6zcXx0XHqLsFJLaUU2j5BDiBAHig=YRoBC_YVq7VJGvzBEA@mail.gmail.com
      7fcdb5e0
    • Peter Eisentraut's avatar
      doc: Update section on NFS · 60bbf075
      Peter Eisentraut authored
      The old section was ancient and didn't seem very helpful.  Here, we
      add some concrete advice on particular mount options.
      Reviewed-by: default avatarJoe Conway <mail@joeconway.com>
      Discussion: https://www.postgresql.org/message-id/flat/e90f24bb-5423-6abb-58ec-501176eb4afc%402ndquadrant.com
      60bbf075
    • Etsuro Fujita's avatar
      Add FDW documentation notes about insert and update tuple routing and COPY. · 90fca7a3
      Etsuro Fujita authored
      Author: Laurenz Albe and Etsuro Fujita
      Reviewed-by: Laurenz Albe and Amit Langote
      Backpatch-through: 11 where support for that by FDWs was added
      Discussion: https://postgr.es/m/bf36a0288e8f31b4f2f40952e225bf892dc1ffc5.camel@cybertec.at
      90fca7a3
  4. 25 Apr, 2019 5 commits
    • Peter Geoghegan's avatar
      Sanitize line pointers within contrib/amcheck. · a9ce839a
      Peter Geoghegan authored
      Adopt a more defensive approach to accessing index tuples in
      contrib/amcheck: verify that each line pointer looks sane before
      accessing associated tuple using pointer arithmetic based on line
      pointer's offset.  This avoids undefined behavior and assertion failures
      in cases where line pointers are corrupt.
      
      Issue spotted following a complaint about an assertion failure by
      Grigory Smolkin, which involved a test harness that deliberately
      corrupts indexes.
      
      This is arguably a bugfix, but no backpatch given the lack of field
      reports beyond Grigory's.
      
      Discussion: https://postgr.es/m/CAH2-WzmkurhCqnyLHxk0VkOZqd49+ZZsp1xAJOg7j2x7dmp_XQ@mail.gmail.com
      a9ce839a
    • Alvaro Herrera's avatar
      Fix partitioned index attachment · 05b38c7e
      Alvaro Herrera authored
      When an existing index in a partition is attached to a new index on
      its parent, we forgot to set the "relispartition" flag correctly, which
      meant that it was not possible to find the index in various operations,
      such as adding a foreign key constraint that references that partitioned
      table.  One of four places that was assigning the parent index was
      forgetting to do that, so fix by shifting responsibility of updating the
      flag to the routine that changes the parent.
      
      Author: Amit Langote, Álvaro Herrera
      Reported-by: Hubert "depesz" Lubaczewski
      Discussion: https://postgr.es/m/CA+HiwqHMsRtRYRWYTWavKJ8x14AFsv7bmAV46mYwnfD3vy8goQ@mail.gmail.com
      05b38c7e
    • Fujii Masao's avatar
      Fix file path in comment. · c247ae09
      Fujii Masao authored
      c247ae09
    • Fujii Masao's avatar
      Fix function names in comments. · 978b032d
      Fujii Masao authored
      Commit 3eb77eba renamed some functions, but forgot to
      update some comments referencing to those functions.
      This commit fixes those function names in the comments.
      
      Kyotaro Horiguchi
      978b032d
    • Alvaro Herrera's avatar
      Fix tablespace inheritance for partitioned rels · 87259588
      Alvaro Herrera authored
      Commit ca410302 left a few loose ends.  The most important one
      (broken pg_dump output) is already fixed by virtue of commit
      3b23552a, but some things remained:
      
      * When ALTER TABLE rewrites tables, the indexes must remain in the
        tablespace they were originally in.  This didn't work because
        index recreation during ALTER TABLE runs manufactured SQL (yuck),
        which runs afoul of default_tablespace in competition with the parent
        relation tablespace.  To fix, reset default_tablespace to the empty
        string temporarily, and add the TABLESPACE clause as appropriate.
      
      * Setting a partitioned rel's tablespace to the database default is
        confusing; if it worked, it would direct the partitions to that
        tablespace regardless of default_tablespace.  But in reality it does
        not work, and making it work is a larger project.  Therefore, throw
        an error when this condition is detected, to alert the unwary.
      
      Add some docs and tests, too.
      
      Author: Álvaro Herrera
      Discussion: https://postgr.es/m/CAKJS1f_1c260nOt_vBJ067AZ3JXptXVRohDVMLEBmudX1YEx-A@mail.gmail.com
      87259588
  5. 24 Apr, 2019 5 commits
    • Alvaro Herrera's avatar
      Make pg_dump emit ATTACH PARTITION instead of PARTITION OF · 3b23552a
      Alvaro Herrera authored
      Using PARTITION OF can result in column ordering being changed from the
      database being dumped, if the partition uses a column layout different
      from the parent's.  It's not pg_dump's job to editorialize on table
      definitions, so this is not acceptable; back-patch all the way back to
      pg10, where partitioned tables where introduced.
      
      This change also ensures that partitions end up in the correct
      tablespace, if different from the parent's; this is an oversight in
      ca410302 (in pg12 only).  Partitioned indexes (in pg11) don't have
      this problem, because they're already created as independent indexes and
      attached to their parents afterwards.
      
      This change also has the advantage that the partition is restorable from
      the dump (as a standalone table) even if its parent table isn't
      restored.
      
      Author: David Rowley
      Reviewed-by: Álvaro Herrera
      Discussion: https://postgr.es/m/CAKJS1f_1c260nOt_vBJ067AZ3JXptXVRohDVMLEBmudX1YEx-A@mail.gmail.com
      Discussion: https://postgr.es/m/20190423185007.GA27954@alvherre.pgsql
      3b23552a
    • Tom Lane's avatar
      Fix some minor postmaster-state-machine issues. · 0fae8462
      Tom Lane authored
      In sigusr1_handler, don't ignore PMSIGNAL_ADVANCE_STATE_MACHINE based
      on pmState.  The restriction is unnecessary (PostmasterStateMachine
      should work in any state), not future-proof (since it makes too many
      assumptions about why the signal might be sent), and broken even today
      because a race condition can make it necessary to respond to the signal
      in PM_WAIT_READONLY state.  The race condition seems unlikely, but
      if it did happen, a hot-standby postmaster could fail to shut down
      after receiving a smart-shutdown request.
      
      In MaybeStartWalReceiver, don't clear the WalReceiverRequested flag
      if the fork attempt fails.  Leaving it set allows us to try
      again in future iterations of the postmaster idle loop.  (The startup
      process would eventually send a fresh request signal, but this change
      may allow us to retry the fork sooner.)
      
      Remove an obsolete comment and unnecessary test in
      PostmasterStateMachine's handling of PM_SHUTDOWN_2 state.  It's not
      possible to have a live walreceiver in that state, and AFAICT has not
      been possible since commit 5e85315e.  This isn't a live bug, but the
      false comment is quite confusing to readers.
      
      In passing, rearrange sigusr1_handler's CheckPromoteSignal tests so that
      we don't uselessly perform stat() calls that we're going to ignore the
      results of.
      
      Add some comments clarifying the behavior of MaybeStartWalReceiver;
      I very nearly rearranged it in a way that'd reintroduce the race
      condition fixed in e5d494d7.  Mea culpa for not commenting that
      properly at the time.
      
      Back-patch to all supported branches.  The PMSIGNAL_ADVANCE_STATE_MACHINE
      change is the only one of even minor significance, but we might as well
      keep this code in sync across branches.
      
      Discussion: https://postgr.es/m/9001.1556046681@sss.pgh.pa.us
      0fae8462
    • Alvaro Herrera's avatar
      Unify error messages · 0a999e12
      Alvaro Herrera authored
      ... for translatability purposes.
      0a999e12
    • Etsuro Fujita's avatar
      postgres_fdw: Fix incorrect handling of row movement for remote partitions. · 5c470491
      Etsuro Fujita authored
      Commit 3d956d95 added support for update row movement in postgres_fdw.
      This patch fixes the following issues introduced by that commit:
      
      * When a remote partition chosen to insert routed rows into was also an
        UPDATE subplan target rel that would be updated later, the UPDATE that
        used a direct modification plan modified those routed rows incorrectly
        because those routed rows were visible to the later UPDATE command.
        The right fix for this would be to have some way in postgres_fdw in
        which the later UPDATE command ignores those routed rows, but it seems
        hard to do so with the current infrastructure.  For now throw an error
        in that case.
      
      * When a remote partition chosen to insert routed rows into was also an
        UPDATE subplan target rel, fmstate created for the UPDATE that used a
        non-direct modification plan was mistakenly overridden by another
        fmstate created for inserting those routed rows into the partition.
        This caused 1) server crash when the partition would be updated later,
        and 2) resource leak when the partition had been already updated.  To
        avoid that, adjust the treatment of the fmstate for the inserting.  As
        for #1, since we would also have the incorrectness issue as mentioned
        above, error out in that case as well.
      
      Update the docs to mention that postgres_fdw currently does not handle
      the case where a remote partition chosen to insert a routed row into is
      also an UPDATE subplan target rel that will be updated later.
      
      Author: Amit Langote and Etsuro Fujita
      Reviewed-by: Amit Langote
      Backpatch-through: 11 where row movement in postgres_fdw was added
      Discussion: https://postgr.es/m/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp
      5c470491
    • Andres Freund's avatar
      Allow pg_class xid & multixid horizons to not be set. · fdc7efcc
      Andres Freund authored
      This allows table AMs that don't need these horizons. This was already
      documented in the tableam relation_set_new_filenode callback, but an
      assert prevented if from actually working (the test AM code contained
      the change itself). Defang the asserts in the general code, and move
      the stronger ones into heap AM.
      
      Relatedly, after CLUSTER/VACUUM, we'd always assign a relfrozenxid /
      relminmxid. Change the table_relation_copy_for_cluster() interface to
      allow the AM to overwrite the horizons that get set on the pg_class
      entry.  This'd also in the future allow AMs like heap to compute a
      relfrozenxid during rewrite that's the table's actual minimum rather
      than a pre-determined value.  Arguably it'd have been better to move
      the whole computation / setting of those values into the callback, but
      it seems likely that for other reasons it'd be better to be able to
      use one value to vacuum/cluster multiple tables (e.g. a toast's
      horizon shouldn't be different than the table's).
      
      Reported-By: Heikki Linnakangas
      Author: Andres Freund
      Discussion: https://postgr.es/m/9a7fb9cc-2419-5db7-8840-ddc10c93f122@iki.fi
      fdc7efcc
  6. 23 Apr, 2019 6 commits
    • Tom Lane's avatar
      Repair assorted issues in locale data extraction. · 7ad1cd31
      Tom Lane authored
      cache_locale_time (extraction of LC_TIME-related info) had never been
      taught the lessons we previously learned about extraction of info related
      to LC_MONETARY and LC_NUMERIC.  Specifically, commit 95a777c6 taught
      PGLC_localeconv() that data coming out of localeconv() was in an encoding
      determined by the relevant locale, but we didn't realize that there's a
      similar issue with strftime().  And commit a4930e7c hardened
      PGLC_localeconv() against errors occurring partway through, but failed
      to do likewise for cache_locale_time().  So, rearrange the latter
      function to perform encoding conversion and not risk failure while
      it's got the locales set to temporary values.
      
      This time around I also changed PGLC_localeconv() to treat it as FATAL
      if it can't restore the previous settings of the locale values.  There
      is no reason (except possibly OOM) for that to fail, and proceeding with
      the wrong locale values seems like a seriously bad idea --- especially
      on Windows where we have to also temporarily change LC_CTYPE.  Also,
      protect against the possibility that we can't identify the codeset
      reported for LC_MONETARY or LC_NUMERIC; rather than just failing,
      try to validate the data without conversion.
      
      The user-visible symptom this fixes is that if LC_TIME is set to a locale
      name that implies an encoding different from the database encoding,
      non-ASCII localized day and month names would be retrieved in the wrong
      encoding, leading to either unexpected encoding-conversion error reports
      or wrong output from to_char().  The other possible failure modes are
      unlikely enough that we've not seen reports of them, AFAIK.
      
      The encoding conversion problems do not manifest on Windows, since
      we'd already created special-case code to handle that issue there.
      
      Per report from Juan José Santamaría Flecha.  Back-patch to all
      supported versions.
      
      Juan José Santamaría Flecha and Tom Lane
      
      Discussion: https://postgr.es/m/CAC+AXB22So5aZm2vZe+MChYXec7gWfr-n-SK-iO091R0P_1Tew@mail.gmail.com
      7ad1cd31
    • Tom Lane's avatar
      Remove useless comment. · e0fb4c9d
      Tom Lane authored
      Commit e439c6f0 removed IndexStmt.relationId, but not the comment
      that had been added to explain it.  Said comment was therefore
      very confusing.
      e0fb4c9d
    • Peter Geoghegan's avatar
      Prevent O(N^2) unique index insertion edge case. · 9b109262
      Peter Geoghegan authored
      Commit dd299df8 made nbtree treat heap TID as a tiebreaker column,
      establishing the principle that there is only one correct location (page
      and page offset number) for every index tuple, no matter what.
      Insertions of tuples into non-unique indexes proceed as if heap TID
      (scan key's scantid) is just another user-attribute value, but
      insertions into unique indexes are more delicate.  The TID value in
      scantid must initially be omitted to ensure that the unique index
      insertion visits every leaf page that duplicates could be on.  The
      scantid is set once again after unique checking finishes successfully,
      which can force _bt_findinsertloc() to step right one or more times, to
      locate the leaf page that the new tuple must be inserted on.
      
      Stepping right within _bt_findinsertloc() was assumed to occur no more
      frequently than stepping right within _bt_check_unique(), but there was
      one important case where that assumption was incorrect: inserting a
      "duplicate" with NULL values.  Since _bt_check_unique() didn't do any
      real work in this case, it wasn't appropriate for _bt_findinsertloc() to
      behave as if it was finishing off a conventional unique insertion, where
      any existing physical duplicate must be dead or recently dead.
      _bt_findinsertloc() might have to grovel through a substantial portion
      of all of the leaf pages in the index to insert a single tuple, even
      when there were no dead tuples.
      
      To fix, treat insertions of tuples with NULLs into a unique index as if
      they were insertions into a non-unique index: never unset scantid before
      calling _bt_search() to descend the tree, and bypass _bt_check_unique()
      entirely.  _bt_check_unique() is no longer responsible for incoming
      tuples with NULL values.
      
      Discussion: https://postgr.es/m/CAH2-Wzm08nr+JPx4jMOa9CGqxWYDQ-_D4wtPBiKghXAUiUy-nQ@mail.gmail.com
      9b109262
    • Tom Lane's avatar
      Avoid order-of-execution problems with ALTER TABLE ADD PRIMARY KEY. · f4a3fdfb
      Tom Lane authored
      Up to now, DefineIndex() was responsible for adding attnotnull constraints
      to the columns of a primary key, in any case where it hadn't been
      convenient for transformIndexConstraint() to mark those columns as
      is_not_null.  It (or rather its minion index_check_primary_key) did this
      by executing an ALTER TABLE SET NOT NULL command for the target table.
      
      The trouble with this solution is that if we're creating the index due
      to ALTER TABLE ADD PRIMARY KEY, and the outer ALTER TABLE has additional
      sub-commands, the inner ALTER TABLE's operations executed at the wrong
      time with respect to the outer ALTER TABLE's operations.  In particular,
      the inner ALTER would perform a validation scan at a point where the
      table's storage might be inconsistent with its catalog entries.  (This is
      on the hairy edge of being a security problem, but AFAICS it isn't one
      because the inner scan would only be interested in the tuples' null
      bitmaps.)  This can result in unexpected failures, such as the one seen
      in bug #15580 from Allison Kaptur.
      
      To fix, let's remove the attempt to do SET NOT NULL from DefineIndex(),
      reducing index_check_primary_key's role to verifying that the columns are
      already not null.  (It shouldn't ever see such a case, but it seems wise
      to keep the check for safety.)  Instead, make transformIndexConstraint()
      generate ALTER TABLE SET NOT NULL subcommands to be executed ahead of
      the ADD PRIMARY KEY operation in every case where it can't force the
      column to be created already-not-null.  This requires only minor surgery
      in parse_utilcmd.c, and it makes for a much more satisfying spec for
      transformIndexConstraint(): it's no longer having to take it on faith
      that someone else will handle addition of NOT NULL constraints.
      
      To make that work, we have to move the execution of AT_SetNotNull into
      an ALTER pass that executes ahead of AT_PASS_ADD_INDEX.  I moved it to
      AT_PASS_COL_ATTRS, and put that after AT_PASS_ADD_COL to avoid failure
      when the column is being added in the same command.  This incidentally
      fixes a bug in the only previous usage of AT_PASS_COL_ATTRS, for
      AT_SetIdentity: it didn't work either for a newly-added column.
      
      Playing around with this exposed a separate bug in ALTER TABLE ONLY ...
      ADD PRIMARY KEY for partitioned tables.  The intent of the ONLY modifier
      in that context is to prevent doing anything that would require holding
      lock for a long time --- but the implied SET NOT NULL would recurse to
      the child partitions, and do an expensive validation scan for any child
      where the column(s) were not already NOT NULL.  To fix that, invent a
      new ALTER subcommand AT_CheckNotNull that just insists that a child
      column be already NOT NULL, and apply that, not AT_SetNotNull, when
      recursing to children in this scenario.  This results in a slightly laxer
      definition of ALTER TABLE ONLY ... SET NOT NULL for partitioned tables,
      too: that command will now work as long as all children are already NOT
      NULL, whereas before it just threw up its hands if there were any
      partitions.
      
      In passing, clean up the API of generateClonedIndexStmt(): remove a
      useless argument, ensure that the output argument is not left undefined,
      update the header comment.
      
      A small side effect of this change is that no-such-column errors in ALTER
      TABLE ADD PRIMARY KEY now produce a different message that includes the
      table name, because they are now detected by the SET NOT NULL step which
      has historically worded its error that way.  That seems fine to me, so
      I didn't make any effort to avoid the wording change.
      
      The basic bug #15580 is of very long standing, and these other bugs
      aren't new in v12 either.  However, this is a pretty significant change
      in the way ALTER TABLE ADD PRIMARY KEY works.  On balance it seems best
      not to back-patch, at least not till we get some more confidence that
      this patch has no new bugs.
      
      Patch by me, but thanks to Jie Zhang for a preliminary version.
      
      Discussion: https://postgr.es/m/15580-d1a6de5a3d65da51@postgresql.org
      Discussion: https://postgr.es/m/1396E95157071C4EBBA51892C5368521017F2E6E63@G08CNEXMBPEKD02.g08.fujitsu.local
      f4a3fdfb
    • Tom Lane's avatar
      Don't request pretty-printed output from xmlNodeDump(). · c06e3550
      Tom Lane authored
      xml.c passed format = 1 to xmlNodeDump(), resulting in sometimes getting
      extra whitespace (newlines + spaces) in the output.  We don't really want
      that, first because whitespace might be semantically significant in some
      XML uses, and second because it happens only very inconsistently.  Only
      one case in our regression tests is affected.
      
      This potentially affects the results of xpath() and the XMLTABLE construct,
      when emitting nodeset values.
      
      Note that the older code in contrib/xml2 doesn't do this; it seems
      to have been an aboriginal bad decision in commit ea3b212f.
      
      While this definitely seems like a bug to me, the small number of
      complaints to date argues against back-patching a behavioral change.
      Hence, fix in HEAD only, at least for now.
      
      Per report from Jean-Marc Voillequin.
      
      Discussion: https://postgr.es/m/1EC8157EB499BF459A516ADCF135ADCE3A23A9CA@LON-WGMSX712.ad.moodys.net
      c06e3550
    • Michael Paquier's avatar
      Fix detection of passwords hashed with MD5 or SCRAM-SHA-256 · ccae190b
      Michael Paquier authored
      This commit fixes a couple of issues related to the way password
      verifiers hashed with MD5 or SCRAM-SHA-256 are detected, leading to
      being able to store in catalogs passwords which do not follow the
      supported hash formats:
      - A MD5-hashed entry was checked based on if its header uses "md5" and
      if the string length matches what is expected.  Unfortunately the code
      never checked if the hash only used hexadecimal characters, as reported
      by Tom Lane.
      - A SCRAM-hashed entry was checked based on only its header, which
      should be "SCRAM-SHA-256$", but it never checked for any fields
      afterwards, as reported by Jonathan Katz.
      
      Backpatch down to v10, which is where SCRAM has been introduced, and
      where password verifiers in plain format have been removed.
      
      Author: Jonathan Katz
      Reviewed-by: Tom Lane, Michael Paquier
      Discussion: https://postgr.es/m/016deb6b-1f0a-8e9f-1833-a8675b170aa9@postgresql.org
      Backpatch-through: 10
      ccae190b
  7. 22 Apr, 2019 2 commits
  8. 21 Apr, 2019 1 commit
    • Tomas Vondra's avatar
      Fix mvdistinct and dependencies size calculations · d08c44f7
      Tomas Vondra authored
      The formulas used to calculate size while (de)serializing mvndistinct
      and functional dependencies were based on offset() of the structs. But
      that is incorrect, because the structures are not copied directly, we
      we copy the individual fields directly.
      
      At the moment this works fine, because there is no alignment padding
      on any platform we support. But it might break if we ever added some
      fields into any of the structs, for example. It's also confusing.
      
      Fixed by reworking the macros to directly sum sizes of serialized
      fields. The macros are now useful only for serialiation, so there is
      no point in keeping them in the public header file. So make them
      private by moving them to the .c files.
      
      Also adds a couple more asserts to check the serialization, and fixes
      an incorrect allocation of MVDependency instead of (MVDependency *).
      
      Reported-By: Tom Lane
      Discussion: https://postgr.es/m/29785.1555365602@sss.pgh.pa.us
      d08c44f7
  9. 20 Apr, 2019 2 commits
    • Bruce Momjian's avatar
      docs: reorder collation regression test order in paragraph · bfbc150e
      Bruce Momjian authored
      Backpatch-through: 10
      bfbc150e
    • Stephen Frost's avatar
      GSSAPI: Improve documentation and tests · eb882a1b
      Stephen Frost authored
      The GSSAPI encryption patch neglected to update the protocol
      documentation to describe how to set up a GSSAPI encrypted connection
      from a client to the server, so fix that by adding the appropriate
      documentation to protocol.sgml.
      
      The tests added for encryption support were overly long and couldn't be
      run in parallel due to race conditions; this was largely because each
      test was setting up its own KDC to perform the tests.  Instead, merge
      the authentication tests and the encryption tests into the original
      test, where we only create one KDC to run the tests with.  Also, have
      the tests check what the server's opinion is of the connection and if it
      was GSS authenticated or encrypted using the pg_stat_gssapi view.
      
      In passing, fix the libpq label for GSSENC-Mode to be consistent with
      the "PGGSSENCMODE" environment variable.
      
      Missing protocol documentation pointed out by Michael Paquier.
      Issues with the tests pointed out by Tom Lane and Peter Eisentraut.
      
      Refactored tests and added documentation by me.
      
      Reviewed by Robbie Harwood (protocol documentation) and Michael Paquier
      (rework of the tests).
      eb882a1b
  10. 19 Apr, 2019 1 commit
    • Andres Freund's avatar
      Fix slot type issue for fuzzy distance index scan over out-of-core table AM. · b8b94ea1
      Andres Freund authored
      For amcanreorderby scans the nodeIndexscan.c's reorder queue holds
      heap tuples, but the underlying table likely does not. Before this fix
      we'd return different types of slots, depending on whether the tuple
      came from the reorder queue, or from the index + table.
      
      While that could be fixed by signalling that the node doesn't return a
      fixed type of slot, it seems better to instead remove the separate
      slot for the reorder queue, and use ExecForceStoreHeapTuple() to store
      tuples from the queue. It's not particularly common to need
      reordering, after all.
      
      This reverts most of the iss_ReorderQueueSlot related changes to
      nodeIndexscan.c made in 1a0586de, except that now
      ExecForceStoreHeapTuple() is used instead of ExecStoreHeapTuple().
      
      Noticed when testing zheap against the in-core version of tableam.
      
      Author: Andres Freund
      b8b94ea1