1. 17 Jul, 2020 6 commits
  2. 16 Jul, 2020 2 commits
    • Andrew Dunstan's avatar
      Enable almost all TAP tests involving symlinks on Windows · d66b23b0
      Andrew Dunstan authored
      Windows has junction points which function as symbolic links for
      directories. This patch introduces a new function TestLib::dir_symlink()
      which creates a junction point on Windows and a standard Unix type
      symbolic link elsewhere.
      
      The function TestLib::perl2host is also modified, first to use cygpath
      where it's available (e.g. msys2) and second to allow it to succeed if
      the gandparent directory exists but the parent does not.
      
      Given these changes the only symlink tests that need to be skipped on
      Windows are those related to permissions or to use of readlink. The
      relevant tests for pg_basebackup and pg_rewind are therefore adjusted
      accordingly.
      
      Andrew Dunstan, reviewed by Peter Eisentraut and Michael Paquier.
      
      Discussion: https://postgr.es/m/c50a646c-d9bb-7c62-a4bf-8256ff6ff338@2ndquadrant.com
      d66b23b0
    • Michael Paquier's avatar
      Switch pg_test_fsync to use binary mode on Windows · 932f9fb5
      Michael Paquier authored
      pg_test_fsync has always opened files using the text mode on Windows, as
      this is the default mode used if not enforced by _setmode().
      
      This fixes a failure when running pg_test_fsync down to 12 because
      O_DSYNC and the text mode are not able to work together nicely.  We
      fixed the handling of O_DSYNC in 12~ for the tool by switching to the
      concurrent-safe version of fopen() in src/port/ with 0ba06e0b.  And
      40cfe860, by enforcing the text mode for compatibility reasons if O_TEXT
      or O_BINARY are not specified by the caller, broke pg_test_fsync.  For
      all versions, this avoids any translation overhead, and pg_test_fsync
      should test binary writes, so it is a gain in all cases.
      
      Note that O_DSYNC is still not handled correctly in ~11, leading to
      pg_test_fsync to show insanely high numbers for open_datasync() (using
      this property it is easy to notice that the binary mode is much
      faster).  This would require a backpatch of 0ba06e0b and 40cfe860, which
      could potentially break existing applications, so this is left out.
      
      There are no TAP tests for this tool yet, so I have checked all builds
      manually using MSVC.  We could invent a new option to run a single
      transaction instead of using a duration of 1s to make the tests a
      maximum short, but this is left as future work.
      
      Thanks to Bruce Momjian for the discussion.
      
      Reported-by: Jeff Janes
      Author: Michael Paquier
      Discussion: https://postgr.es/m/16526-279ded30a230d275@postgresql.org
      Backpatch-through: 9.5
      932f9fb5
  3. 15 Jul, 2020 4 commits
    • Peter Eisentraut's avatar
      pg_dump: Reorganize dumpFunc() and dumpAgg() · ed2c7f65
      Peter Eisentraut authored
      Similar to daa9fe8a, instead of
      repeating the almost same large query in each version branch, use one
      query and add a few columns to the SELECT list depending on the
      version.  This saves a lot of duplication.
      Reviewed-by: default avatarFabien COELHO <coelho@cri.ensmp.fr>
      Discussion: https://www.postgresql.org/message-id/flat/6594334b-40fd-14f1-6bc5-877afa3feed5@2ndquadrant.com
      ed2c7f65
    • Michael Paquier's avatar
      Fix handling of missing files when using pg_rewind with online source · 1d09fb1f
      Michael Paquier authored
      When working with an online source cluster, pg_rewind gets a list of all
      the files in the source data directory using a WITH RECURSIVE query,
      returning a NULL result for a file's metadata if it gets removed between
      the moment it is listed in a directory and the moment its metadata is
      obtained with pg_stat_file() (say a recycled WAL segment).  The query
      result was processed in such a way that for each tuple we checked only
      that the first file's metadata was NULL.  This could have two
      consequences, both resulting in a failure of the rewind:
      - If the first tuple referred to a removed file, all files from the
      source would be ignored.
      - Any file actually missing would not be considered as such.
      
      While on it, rework slightly the code so as no values are saved if we
      know that a file is going to be skipped.
      
      Issue introduced by b36805f3, so backpatch down to 9.5.
      
      Author: Justin Pryzby, Michael Paquier
      Reviewed-by: Daniel Gustafsson, Masahiko Sawada
      Discussion: https://postgr.es/m/20200713061010.GC23581@telsasoft.com
      Backpatch-through: 9.5
      1d09fb1f
    • Michael Paquier's avatar
      Fix compilation failure with sepgsql · e9491373
      Michael Paquier authored
      One change for getObjectIdentity() has been missed in 2a10fdc4, causing
      the module to not compile properly.  This was actually the only problem,
      and it happens that it is easy enough to check the compilation of the
      module on Debian after installing libselinux1-dev.
      
      Per buildfarm member rhinoceros.
      e9491373
    • Michael Paquier's avatar
      Eliminate cache lookup errors in SQL functions for object addresses · 2a10fdc4
      Michael Paquier authored
      When using the following functions, users could see various types of
      errors of the type "cache lookup failed for OID XXX" with elog(), that
      can only be used for internal errors:
      * pg_describe_object()
      * pg_identify_object()
      * pg_identify_object_as_address()
      
      The set of APIs managing object addresses for all object types are made
      smarter by gaining a new argument "missing_ok" that allows any caller to
      control if an error is raised or not on an undefined object.  The SQL
      functions listed above are changed to handle the case where an object is
      missing.
      
      Regression tests are added for all object types for the cases where
      these are undefined.  Before this commit, these cases failed with cache
      lookup errors, and now they basically return NULL (minus the name of the
      object type requested).
      
      Author: Michael Paquier
      Reviewed-by: Aleksander Alekseev, Dmitry Dolgov, Daniel Gustafsson,
      Álvaro Herrera, Kyotaro Horiguchi
      Discussion: https://postgr.es/m/CAB7nPqSZxrSmdHK-rny7z8mi=EAFXJ5J-0RbzDw6aus=wB5azQ@mail.gmail.com
      2a10fdc4
  4. 14 Jul, 2020 7 commits
    • Tom Lane's avatar
      Fix bitmap AND/OR scans on the inside of a nestloop partition-wise join. · 689696c7
      Tom Lane authored
      reparameterize_path_by_child() failed to reparameterize BitmapAnd
      and BitmapOr paths.  This matters only if such a path is chosen as
      the inside of a nestloop partition-wise join, where we have to pass
      in parameters from the outside of the nestloop.  If that did happen,
      we generated a bad plan that would likely lead to crashes at execution.
      
      This is not entirely reparameterize_path_by_child()'s fault though;
      it's the victim of an ancient decision (my ancient decision, I think)
      to not bother filling in param_info in BitmapAnd/Or path nodes.  That
      caused the function to believe that such nodes and their children
      contain no parameter references and so need not be processed.
      
      In hindsight that decision looks pretty penny-wise and pound-foolish:
      while it saves a few cycles during path node setup, we do commonly
      need the information later.  In particular, by reversing the decision
      and requiring valid param_info data in all nodes of a bitmap path
      tree, we can get rid of indxpath.c's get_bitmap_tree_required_outer()
      function, which computed the data on-demand.  It's not unlikely that
      that nets out as a savings of cycles in many scenarios.  A couple
      of other things in indxpath.c can be simplified as well.
      
      While here, get rid of some cases in reparameterize_path_by_child()
      that are visibly dead or useless, given that we only care about
      reparameterizing paths that can be on the inside of a parameterized
      nestloop.  This case reminds one of the maxim that untested code
      probably does not work, so I'm unwilling to leave unreachable code
      in this function.  (I did leave the T_Gather case in place even
      though it's not reached in the regression tests.  It's not very
      clear to me when the planner might prefer to put Gather below
      rather than above a nestloop, but at least in principle the case
      might be interesting.)
      
      Per bug #16536, originally from Arne Roland but with a test case
      by Andrew Gierth.  Back-patch to v11 where this code came in.
      
      Discussion: https://postgr.es/m/16536-2213ee0b3aad41fd@postgresql.org
      689696c7
    • Peter Eisentraut's avatar
      Fix -Wcast-function-type warnings · de8feb1f
      Peter Eisentraut authored
      Three groups of issues needed to be addressed:
      
      load_external_function() and related functions returned PGFunction,
      even though not necessarily all callers are looking for a function of
      type PGFunction.  Since these functions are really just wrappers
      around dlsym(), change to return void * just like dlsym().
      
      In dynahash.c, we are using strlcpy() where a function with a
      signature like memcpy() is expected.  This should be safe, as the new
      comment there explains, but the cast needs to be augmented to avoid
      the warning.
      
      In PL/Python, methods all need to be cast to PyCFunction, per Python
      API, but this now runs afoul of these warnings.  (This issue also
      exists in core CPython.)
      
      To fix the second and third case, we add a new type pg_funcptr_t that
      is defined specifically so that gcc accepts it as a special function
      pointer that can be cast to any other function pointer without the
      warning.
      
      Also add -Wcast-function-type to the standard warning flags, subject
      to configure check.
      Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Discussion: https://www.postgresql.org/message-id/flat/1e97628e-6447-b4fd-e230-d109cec2d584%402ndquadrant.com
      de8feb1f
    • David Rowley's avatar
      Add comment to explain an unused function parameter · 101f903e
      David Rowley authored
      Removing the unused 'miinfo' parameter has been raised a couple of times
      now.  It was decided in the 2nd discussion below that we're going to leave
      it alone.  It seems like it might be useful to add a comment to mention
      this fact so that nobody wastes any time in the future proposing its
      removal again.
      
      Discussion: https://postgr.es/m/CAApHDvpCf-qR5HC1rXskUM4ToV+3YDb4-n1meY=vpAHsRS_1PA@mail.gmail.com
      Discussion: https://postgr.es/m/CAE9k0P%3DFvcDswnSVtRpSyZMpcAWC%3DGp%3DifZ0HdfPaRQ%3D__LBtw%40mail.gmail.com
      101f903e
    • David Rowley's avatar
      Fix timing issue with ALTER TABLE's validate constraint · f1fcf2d3
      David Rowley authored
      An ALTER TABLE to validate a foreign key in which another subcommand
      already caused a pending table rewrite could fail due to ALTER TABLE
      attempting to validate the foreign key before the actual table rewrite
      takes place.  This situation could result in an error such as:
      
      ERROR:  could not read block 0 in file "base/nnnnn/nnnnn": read only 0 of 8192 bytes
      
      The failure here was due to the SPI call which validates the foreign key
      trying to access an index which is yet to be rebuilt.
      
      Similarly, we also incorrectly tried to validate CHECK constraints before
      the heap had been rewritten.
      
      The fix for both is to delay constraint validation until phase 3, after
      the table has been rewritten.  For CHECK constraints this means a slight
      behavioral change.  Previously ALTER TABLE VALIDATE CONSTRAINT on
      inheritance tables would be validated from the bottom up.  This was
      different from the order of evaluation when a new CHECK constraint was
      added.  The changes made here aligns the VALIDATE CONSTRAINT evaluation
      order for inheritance tables to be the same as ADD CONSTRAINT, which is
      generally top-down.
      
      Reported-by: Nazli Ugur Koyluoglu, using SQLancer
      Discussion: https://postgr.es/m/CAApHDvp%3DZXv8wiRyk_0rWr00skhGkt8vXDrHJYXRMft3TjkxCA%40mail.gmail.com
      Backpatch-through: 9.5 (all supported versions)
      f1fcf2d3
    • Michael Paquier's avatar
      Fix some header identifications · b8401c32
      Michael Paquier authored
      The following header files missed the shot:
      - jsonfuncs.h, as of ce0425b1.
      - jsonapi.h, as of beb46990.
      - llvmjit_emit.h as of 7ec0d80c.
      - partdesc.h, as of 1bb5e782.
      
      Author: Jesse Zhang
      Discussion: https://postgr.es/m/CAGf+fX4-8xULEOz09DE2dZGjT+q8VJ--rqfTpvcFwc+A4fc-3Q@mail.gmail.com
      b8401c32
    • Michael Paquier's avatar
      Fix comments related to table AMs · 9168793d
      Michael Paquier authored
      Incorrect function names were referenced.  As this fixes some portions
      of tableam.h, that is mentioned in the docs as something to look at when
      implementing a table AM, backpatch down to 12 where this has been
      introduced.
      
      Author: Hironobu Suzuki
      Discussion: https://postgr.es/m/8fe6d672-28dd-3f1d-7aed-ac2f6d599d3f@interdb.jp
      Backpatch-through: 12
      9168793d
    • Tom Lane's avatar
      Cope with lateral references in the quals of a subquery RTE. · a742ecf9
      Tom Lane authored
      The qual pushdown logic assumed that all Vars in a restriction clause
      must be Vars referencing subquery outputs; but since we introduced
      LATERAL, it's possible for such a Var to be a lateral reference instead.
      This led to an assertion failure in debug builds.  In a non-debug
      build, there might be no ill effects (if qual_is_pushdown_safe decided
      the qual was unsafe anyway), or we could get failures later due to
      construction of an invalid plan.  I've not gone to much length to
      characterize the possible failures, but at least segfaults in the
      executor have been observed.
      
      Given that this has been busted since 9.3 and it took this long for
      anybody to notice, I judge that the case isn't worth going to great
      lengths to optimize.  Hence, fix by just teaching qual_is_pushdown_safe
      that such quals are unsafe to push down, matching the previous behavior
      when it accidentally didn't fail.
      
      Per report from Tom Ellis.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/20200713175124.GQ8220@cloudinit-builder
      a742ecf9
  5. 13 Jul, 2020 7 commits
  6. 12 Jul, 2020 2 commits
    • Michael Paquier's avatar
      Fix test failure with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS · ea3e15d1
      Michael Paquier authored
      Replication origins created by regression tests should have names
      starting with "regress_", and the test introduced in b1e48bbe for commit
      timestamps did not do that.
      
      Per buildfarm member longfin.
      
      Discussion: https://postgr.es/m/20200712122507.GD21680@paquier.xyz
      ea3e15d1
    • Michael Paquier's avatar
      Include replication origins in SQL functions for commit timestamp · b1e48bbe
      Michael Paquier authored
      This includes two changes:
      - Addition of a new function pg_xact_commit_timestamp_origin() able, for
      a given transaction ID, to return the commit timestamp and replication
      origin of this transaction.  An equivalent function existed in
      pglogical.
      - Addition of the replication origin to pg_last_committed_xact().
      
      The commit timestamp manager includes already APIs able to return the
      replication origin of a transaction on top of its commit timestamp, but
      the code paths for replication origins were never stressed as those
      functions have never looked for a replication origin, and the SQL
      functions available have never included this information since their
      introduction in 73c986ad.
      
      While on it, refactor a test of modules/commit_ts/ to use tstzrange() to
      check that a transaction timestamp is within the wanted range, making
      the test a bit easier to read.
      
      Bump catalog version.
      
      Author: Movead Li
      Reviewed-by: Madan Kumar, Michael Paquier
      Discussion: https://postgr.es/m/2020051116430836450630@highgo.ca
      b1e48bbe
  7. 11 Jul, 2020 6 commits
    • Tom Lane's avatar
      Avoid useless buffer allocations during binary COPY FROM. · cd22d3cd
      Tom Lane authored
      The raw_buf and line_buf buffers aren't used when reading binary format,
      so skip allocating them.  raw_buf is 64K so that seems like a worthwhile
      savings.  An unused line_buf only wastes 1K, but as long as we're checking
      it's free to avoid allocating that too.
      
      Bharath Rupireddy, tweaked a bit by me
      
      Discussion: https://postgr.es/m/CALj2ACXcCKaGPY0whowqrJ4OPJvDnTssgpGCzvuFQu5z0CXb-g@mail.gmail.com
      cd22d3cd
    • Tom Lane's avatar
      Avoid trying to restore table ACLs and per-column ACLs in parallel. · ea912530
      Tom Lane authored
      Parallel pg_restore has always supposed that ACL items for different
      objects are independent and can be restored in parallel without
      conflicts.  However, there is one case where this fails: because
      REVOKE on a table is defined to also revoke the privilege(s) at
      column level, we can't restore per-column ACLs till after we restore
      any table-level privileges on their table.  Failure to honor this
      restriction can lead to "tuple concurrently updated" errors during
      parallel restore, or even to the per-column ACLs silently disappearing
      because the table-level REVOKE is executed afterwards.
      
      To fix, add a dependency from each column-level ACL item to its table's
      ACL item, if there is one.  Note that this doesn't fix the hazard
      for pre-existing archive files, only for ones made with a corrected
      pg_dump.  Given that the bug's been there quite awhile without
      field reports, I think this is acceptable.
      
      This requires changing the API of pg_dump's dumpACL() function.
      To keep its argument list from getting even longer, I removed the
      "CatalogId objCatId" argument, which has been unused for ages.
      
      Per report from Justin Pryzby.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/20200706050129.GW4107@telsasoft.com
      ea912530
    • Peter Eisentraut's avatar
    • Michael Paquier's avatar
      Rename field "relkind" to "objtype" for CTAS and ALTER TABLE nodes · cc35d893
      Michael Paquier authored
      "relkind" normally refers to the char field from pg_class.  However, in
      the parse nodes AlterTableStmt and CreateTableAsStmt, "relkind" was used
      for a field of type enum ObjectType, that could refer to other object
      types than those possible for a relkind.  Such fields being usually
      named "objtype", switch the name in both structures to make things more
      consistent.  Note that this led to some confusion in functions that
      also operate on a RangeTableEntry object, which also has a field named
      "relkind".
      
      This naming goes back to commit 09d4e96d, where only OBJECT_TABLE and
      OBJECT_INDEX were used.  This got extended later to use as well
      OBJECT_TYPE with e440e12c, not really a relation kind.
      
      Author: Mark Dilger
      Reviewed-by: Daniel Gustafsson, Álvaro Herrera, Michael Paquier
      Discussion: https://postgr.es/m/609181AE-E399-47C7-9221-856E0F96BF93@enterprisedb.com
      cc35d893
    • Alexander Korotkov's avatar
      Forbid numeric NaN in jsonpath · df646509
      Alexander Korotkov authored
      SQL standard doesn't define numeric Inf or NaN values.  It appears even more
      ridiculous to support then in jsonpath assuming JSON doesn't support these
      values as well.  This commit forbids returning NaN from .double(), which was
      previously allowed.  NaN can't be result of inner-jsonpath computation over
      non-NaNs.  So, we can not expect NaN in the jsonpath output.
      
      Reported-by: Tom Lane
      Discussion: https://postgr.es/m/203949.1591879542%40sss.pgh.pa.us
      Author: Alexander Korotkov
      Reviewed-by: Tom Lane
      Backpatch-through: 12
      df646509
    • Alexander Korotkov's avatar
      Improve error reporting for jsonpath .double() method · 06571811
      Alexander Korotkov authored
      When jsonpath .double() method detects that numeric or string can't be
      converted to double precision, it throws an error.  This commit makes these
      errors explicitly express the reason of failure.
      
      Discussion: https://postgr.es/m/CAPpHfdtqJtiSXkP7tOXez18NxhLUH_-75bL8%3DOce4Ki%2Bbv7V6Q%40mail.gmail.com
      Author: Alexander Korotkov
      Reviewed-by: Tom Lane
      Backpatch-through: 12
      06571811
  8. 10 Jul, 2020 5 commits
  9. 09 Jul, 2020 1 commit
    • Tom Lane's avatar
      Fix pg_current_logfile() to not emit a carriage return on Windows. · 183926da
      Tom Lane authored
      Due to not having our signals straight about CRLF vs. LF line
      termination, the output of pg_current_logfile() included a trailing
      \r on Windows.  To fix, force the file descriptor it uses into text
      mode.
      
      While here, move a couple of local variable declarations to make
      the function's logic clearer.
      
      In v12 and v13, also back-patch the test added by 1c4e88e2 so that
      this function has some test coverage.  However, the 004_logrotate.pl
      test script doesn't exist before v12, and it didn't seem worth adding
      to older branches just for this.
      
      Per report from Thomas Kellerer.  Back-patch to v10 where this
      function was added.
      
      Discussion: https://postgr.es/m/412ae8da-76bb-640f-039a-f3513499e53d@gmx.net
      183926da