1. 04 Nov, 2020 12 commits
    • Tom Lane's avatar
      Improve our ability to regurgitate SQL-syntax function calls. · 40c24bfe
      Tom Lane authored
      The SQL spec calls out nonstandard syntax for certain function calls,
      for example substring() with numeric position info is supposed to be
      spelled "SUBSTRING(string FROM start FOR count)".  We accept many
      of these things, but up to now would not print them in the same format,
      instead simplifying down to "substring"(string, start, count).
      That's long annoyed me because it creates an interoperability
      problem: we're gratuitously injecting Postgres-specific syntax into
      what might otherwise be a perfectly spec-compliant view definition.
      However, the real reason for addressing it right now is to support
      a planned change in the semantics of EXTRACT() a/k/a date_part().
      When we switch that to returning numeric, we'll have the parser
      translate EXTRACT() to some new function name (might as well be
      "extract" if you ask me) and then teach ruleutils.c to reverse-list
      that per SQL spec.  In this way existing calls to date_part() will
      continue to have the old semantics.
      
      To implement this, invent a new CoercionForm value COERCE_SQL_SYNTAX,
      and make the parser insert that rather than COERCE_EXPLICIT_CALL when
      the input has SQL-spec decoration.  (But if the input has the form of
      a plain function call, continue to mark it COERCE_EXPLICIT_CALL, even
      if it's calling one of these functions.)  Then ruleutils.c recognizes
      COERCE_SQL_SYNTAX as a cue to emit SQL call syntax.  It can know
      which decoration to emit using hard-wired knowledge about the
      functions that could be called this way.  (While this solution isn't
      extensible without manual additions, neither is the grammar, so this
      doesn't seem unmaintainable.)  Notice that this solution will
      reverse-list a function call with SQL decoration only if it was
      entered that way; so dump-and-reload will not by itself produce any
      changes in the appearance of views.
      
      This requires adding a CoercionForm field to struct FuncCall.
      (I couldn't resist the temptation to rearrange that struct's
      field order a tad while I was at it.)  FuncCall doesn't appear
      in stored rules, so that change isn't a reason for a catversion
      bump, but I did one anyway because the new enum value for
      CoercionForm fields could confuse old backend code.
      
      Possible future work:
      
      * Perhaps CoercionForm should now be renamed to DisplayForm,
      or something like that, to reflect its more general meaning.
      This'd require touching a couple hundred places, so it's not
      clear it's worth the code churn.
      
      * The SQLValueFunction node type, which was invented partly for
      the same goal of improving SQL-compatibility of view output,
      could perhaps be replaced with regular function calls marked
      with COERCE_SQL_SYNTAX.  It's unclear if this would be a net
      code savings, however.
      
      Discussion: https://postgr.es/m/42b73d2d-da12-ba9f-570a-420e0cce19d9@phystech.edu
      40c24bfe
    • Tom Lane's avatar
      Remove useless entries for aggregate functions from fmgrtab.c. · f21636e5
      Tom Lane authored
      Gen_fmgrtab.pl treated aggregate functions the same as other built-in
      functions, which is wasteful because there is no real need to have
      entries for them in the fmgr_builtins[] table.  Suppressing those
      entries saves about 3KB in the compiled table on my machine; which
      is not a lot but it's not nothing either, considering that that
      table is pretty "hot".  The only outside code change needed is
      that ExecInitWindowAgg() can't be allowed to call fmgr_info_cxt()
      on a plain aggregate function.  But that saves a few cycles anyway.
      
      Having done that, the aggregate_dummy() function is unreferenced
      and might as well be dropped.  Using "aggregate_dummy" as the prosrc
      value for an aggregate is now just a documentation convention not
      something that matters.  There was some discussion of using NULL
      instead to save a few bytes in pg_proc, but we'd have to remove
      prosrc's BKI_FORCE_NOT_NULL marking which doesn't seem a great idea.
      Anyway, it's possible there's client-side code that expects to
      see "aggregate_dummy" there, so I'm loath to change it without a
      strong reason.
      
      Discussion: https://postgr.es/m/533989.1604263665@sss.pgh.pa.us
      f21636e5
    • Fujii Masao's avatar
      Fix segmentation fault that commit ac22929a caused. · 113d3591
      Fujii Masao authored
      Commit ac22929a changed recoveryWakeupLatch so that it's reset to
      NULL at the end of recovery. This change could cause a segmentation fault
      in the buildfarm member 'elver'.
      
      Previously the latch was reset to NULL after calling ShutdownWalRcv().
      But there could be a window between ShutdownWalRcv() and the actual
      exit of walreceiver. If walreceiver set the latch during that window,
      the segmentation fault could happen.
      
      To fix the issue, this commit changes walreceiver so that it sets
      the latch only when the latch has not been reset to NULL yet.
      
      Author: Fujii Masao
      Discussion: https://postgr.es/m/5c1f8a85-747c-7bf9-241e-dd467d8a3586@iki.fi
      113d3591
    • Peter Eisentraut's avatar
      Enable hash partitioning of text arrays · 560564d3
      Peter Eisentraut authored
      hash_array_extended() needs to pass PG_GET_COLLATION() to the hash
      function of the element type.  Otherwise, the hash function of a
      collation-aware data type such as text will error out, since the
      introduction of nondeterministic collation made hash functions require
      a collation, too.
      
      The consequence of this is that before this change, hash partitioning
      using an array over text in the partition key would not work.
      Reviewed-by: default avatarHeikki Linnakangas <hlinnaka@iki.fi>
      Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Reviewed-by: default avatarMichael Paquier <michael@paquier.xyz>
      Discussion: https://www.postgresql.org/message-id/flat/32c1fdae-95c6-5dc6-058a-a90330a3b621%40enterprisedb.com
      560564d3
    • Heikki Linnakangas's avatar
      pg_rewind: Refactor the abstraction to fetch from local/libpq source. · 37d2ff38
      Heikki Linnakangas authored
      This makes the abstraction of a "source" server more clear, by introducing
      a common abstract class, borrowing the object-oriented programming term,
      that represents all the operations that can be done on the source server.
      There are two implementations of it, one for fetching via libpq, and
      another to fetch from a local directory. This adds some code, but makes it
      easier to understand what's going on.
      
      The copy_executeFileMap() and libpq_executeFileMap() functions contained
      basically the same logic, just calling different functions to fetch the
      source files. Refactor so that the common logic is in one place, in a new
      function called perform_rewind().
      
      Reviewed-by: Kyotaro Horiguchi, Soumyadeep Chakraborty
      Discussion: https://www.postgresql.org/message-id/0c5b3783-af52-3ee5-f8fa-6e794061f70d%40iki.fi
      37d2ff38
    • Heikki Linnakangas's avatar
      pg_rewind: Replace the hybrid list+array data structure with simplehash. · f81e97d0
      Heikki Linnakangas authored
      Now that simplehash can be used in frontend code, let's make use of it.
      
      Reviewed-by: Kyotaro Horiguchi, Soumyadeep Chakraborty
      Discussion: https://www.postgresql.org/message-id/0c5b3783-af52-3ee5-f8fa-6e794061f70d%40iki.fi
      f81e97d0
    • Heikki Linnakangas's avatar
      Refactor pg_rewind for more clear decision making. · eb00f1d4
      Heikki Linnakangas authored
      Deciding what to do with each file is now a separate step after all the
      necessary information has been gathered. It is more clear that way.
      Previously, the decision-making was divided between process_source_file()
      and process_target_file(), and it was a bit hard to piece together what
      the overall rules were.
      
      Reviewed-by: Kyotaro Horiguchi, Soumyadeep Chakraborty
      Discussion: https://www.postgresql.org/message-id/0c5b3783-af52-3ee5-f8fa-6e794061f70d%40iki.fi
      eb00f1d4
    • Heikki Linnakangas's avatar
      pg_rewind: Move syncTargetDirectory() to file_ops.c · ffb4e27e
      Heikki Linnakangas authored
      For consistency. All the other low-level functions that operate on the
      target directory are in file_ops.c.
      
      Reviewed-by: Michael Paquier
      Discussion: https://www.postgresql.org/message-id/0c5b3783-af52-3ee5-f8fa-6e794061f70d%40iki.fi
      ffb4e27e
    • Fujii Masao's avatar
      Get rid of the dedicated latch for signaling the startup process. · ac22929a
      Fujii Masao authored
      This commit gets rid of the dedicated latch for signaling the startup
      process in favor of using its procLatch,  since that comports better
      with possible generic signal handlers using that latch.
      
      Commit 1e53fe0e changed background processes so that they use standard
      SIGHUP handler. Like that, this commit also makes the startup process use
      standard SIGHUP handler to simplify the code.
      
      Author: Fujii Masao
      Reviewed-by: Bharath Rupireddy, Michael Paquier
      Discussion: https://postgr.es/m/CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com
      ac22929a
    • Fujii Masao's avatar
      Use standard SIGHUP handler in syslogger. · 02d33229
      Fujii Masao authored
      Commit 1e53fe0e changed background processes so that they use
      standard SIGHUP handler. Like that, this commit makes syslogger use
      standard SIGHUP handler to simplify the code.
      
      Author: Bharath Rupireddy
      Reviewed-by: Fujii Masao
      Discussion: https://postgr.es/m/CALj2ACXPorUqePswDtOeM_s82v9RW32E1fYmOPZ5NuE+TWKj_A@mail.gmail.com
      02d33229
    • Thomas Munro's avatar
      Tolerate version lookup failure for old style Windows locale names. · 9f12a3b9
      Thomas Munro authored
      Accept that we can't get versions for such locale names for now.  Users
      will need to specify the newer language tag format to enable the
      collation versioning feature.  It's not clear that we can do automatic
      conversion from the old style to the new style reliably enough for this
      purpose.
      
      Unfortunately, this means that collation versioning probably won't work
      for the default collation unless you provide something like en-US at
      initdb or CREATE DATABASE time (though, for reasons not yet understood,
      it does seem to work on some systems).  It'd be nice to find a better
      solution, or document this quirk if we settle on it, but this should
      unbreak the 3 failing build farm animals in the meantime.
      Reviewed-by: default avatarDavid Rowley <dgrowleyml@gmail.com>
      Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
      9f12a3b9
    • Michael Paquier's avatar
      Revert pg_relation_check_pages() · e152506a
      Michael Paquier authored
      This reverts the following set of commits, following complaints about
      the lack of portability of the central part of the code in bufmgr.c as
      well as the use of partition mapping locks during page reads:
      c780a7a9
      f2b88396
      b787d4ce
      ce7f772c
      60a51c6b
      
      Per discussion with Andres Freund, Robert Haas and myself.
      
      Bump catalog version.
      
      Discussion: https://postgr.es/m/20201029181729.2nrub47u7yqncsv7@alap3.anarazel.de
      e152506a
  2. 03 Nov, 2020 13 commits
  3. 02 Nov, 2020 13 commits
    • Tom Lane's avatar
      Remove special checks for pg_rewrite.ev_qual and ev_action being NULL. · e1339bfc
      Tom Lane authored
      make_ruledef() and make_viewdef() were coded to cope with possible
      null-ness of these columns, but they've been marked BKI_FORCE_NOT_NULL
      for some time.  So there's not really any need to do more than what
      we do for the other columns of pg_rewrite, i.e. just Assert that
      we got non-null results.
      
      (There is a school of thought that says Asserts aren't the thing
      to do to check for corrupt data, but surely here is not the place
      to start if we want such a policy.)
      
      Also, remove long-dead-if-indeed-it-ever-wasn't-dead handling of
      an empty actions list in make_ruledef().  That's an error case
      and should be treated as such.  (DO INSTEAD NOTHING is represented
      by a CMD_NOTHING Query, not an empty list; cf transformRuleStmt.)
      
      Kyotaro Horiguchi, some changes by me
      
      Discussion: https://postgr.es/m/CAEudQApoA=tMTic6xEPYP_hsNZ8XtToVThK_0x7D_aFQYowq3w@mail.gmail.com
      e1339bfc
    • Tom Lane's avatar
      Rethink the generation rule for fmgroids.h macros. · 8e1f37c0
      Tom Lane authored
      Traditionally, the names of fmgroids.h macros for pg_proc OIDs
      have been constructed from the prosrc field.  But sometimes the
      same C function underlies multiple pg_proc entries, forcing us
      to make an arbitrary choice of which OID to reference; the other
      entries are then not namable via fmgroids.h.  Moreover, we could
      not have macros at all for pg_proc entries that aren't for
      C-coded functions.
      
      Instead, use the proname field, and append the proargtypes field
      (replacing inter-argument spaces with underscores) if proname is
      not unique.  Special-casing unique entries such as F_OIDEQ removes
      the need to change a lot of code.  Indeed, I can only find two
      places in the tree that need to be adjusted; while this changes
      quite a few existing entries in fmgroids.h, few of them are
      referenced from C code.
      
      With this patch, all entries in pg_proc.dat have macros in fmgroids.h.
      
      Discussion: https://postgr.es/m/472274.1604258384@sss.pgh.pa.us
      8e1f37c0
    • Tom Lane's avatar
      Second thoughts on TOAST decompression. · fd299756
      Tom Lane authored
      On detecting a corrupted match tag, pglz_decompress() should just
      summarily return -1.  Breaking out of the loop, as I did in dfc79773,
      doesn't quite guarantee that will happen.  Also, we can use
      unlikely() on that check, just in case it helps.
      
      Backpatch to v13, like the previous patch.
      fd299756
    • Peter Eisentraut's avatar
      Use PG_GETARG_TRANSACTIONID where appropriate · dd26a0ad
      Peter Eisentraut authored
      Some places were using PG_GETARG_UINT32 where PG_GETARG_TRANSACTIONID
      would be more appropriate.  (Of course, they are the same internally,
      so there is no externally visible effect.)  To do that, export
      PG_GETARG_TRANSACTIONID outside of xid.c.  We also export
      PG_RETURN_TRANSACTIONID for symmetry, even though there are currently
      no external users.
      
      Author: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>
      Discussion: https://www.postgresql.org/message-id/flat/d8f6bdd536df403b9b33816e9f7e0b9d@G08CNEXMBPEKD05.g08.fujitsu.local
      dd26a0ad
    • Magnus Hagander's avatar
      Clarify temporary table name shadowing in CREATE TABLE docs · 5b3dca09
      Magnus Hagander authored
      Author: David Johnston
      5b3dca09
    • Thomas Munro's avatar
      Track collation versions for indexes. · 257836a7
      Thomas Munro authored
      Record the current version of dependent collations in pg_depend when
      creating or rebuilding an index.  When accessing the index later, warn
      that the index may be corrupted if the current version doesn't match.
      
      Thanks to Douglas Doole, Peter Eisentraut, Christoph Berg, Laurenz Albe,
      Michael Paquier, Robert Haas, Tom Lane and others for very helpful
      discussion.
      
      Author: Thomas Munro <thomas.munro@gmail.com>
      Author: Julien Rouhaud <rjuju123@gmail.com>
      Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com> (earlier versions)
      Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
      257836a7
    • Thomas Munro's avatar
      Add pg_depend.refobjversion. · cd6f479e
      Thomas Munro authored
      Provide a place for the version of referenced database objects to be
      recorded.  A follow-up commit will use this to record dependencies on
      collation versions for indexes, but similar ideas for other kinds of
      objects have also been mooted.
      
      Author: Thomas Munro <thomas.munro@gmail.com>
      Reviewed-by: default avatarJulien Rouhaud <rjuju123@gmail.com>
      Reviewed-by: default avatarPeter Eisentraut <peter.eisentraut@2ndquadrant.com>
      Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
      cd6f479e
    • Thomas Munro's avatar
      Remove pg_collation.collversion. · 7d1297df
      Thomas Munro authored
      This model couldn't be extended to cover the default collation, and
      didn't have any information about the affected database objects when the
      version changed.  Remove, in preparation for a follow-up commit that
      will add a new mechanism.
      
      Author: Thomas Munro <thomas.munro@gmail.com>
      Reviewed-by: default avatarJulien Rouhaud <rjuju123@gmail.com>
      Reviewed-by: default avatarPeter Eisentraut <peter.eisentraut@2ndquadrant.com>
      Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com
      7d1297df
    • Heikki Linnakangas's avatar
      doc: Mention UNION/ORDER BY etc. keywords in section headers. · 8ef2a5af
      Heikki Linnakangas authored
      Most of the section and sub-section headers in the Queries chapter have
      the keywords literally stated, but neither "Sorting Rows" nor "Combining
      Rows" did. There's no rule that they must be, but it seems like a good
      practice. The keywords will ring a bell to anyone with with even a little
      bit of SQL experience.
      
      David G. Johnston, per suggestion by bilge@scriptfusion.com
      
      Discussion: https://www.postgresql.org/message-id/159981394174.31338.7014519396749859167%40wrigleys.postgresql.org
      8ef2a5af
    • David Rowley's avatar
      Fix unstable partition_prune regression tests · 90d8f1b1
      David Rowley authored
      This was broken recently by a929e17e.  I'd failed to remember that
      parallel tests should have their EXPLAIN output run through the
      explain_parallel_append function so that the output is stable when
      parallel workers fail to start.
      
      fairywren was first to notice.
      
      Reported-by: Michael Paquier
      Discussion: https://postgr.es/m/20201102062951.GB15770@paquier.xyz
      90d8f1b1
    • Michael Paquier's avatar
      Fix some grammar and typos in comments and docs · 8a15e735
      Michael Paquier authored
      The documentation fixes are backpatched down to where they apply.
      
      Author: Justin Pryzby
      Discussion: https://postgr.es/m/20201031020801.GD3080@telsasoft.com
      Backpatch-through: 9.6
      8a15e735
    • Amit Kapila's avatar
      Use Enum for top level logical replication message types. · 644f0d7c
      Amit Kapila authored
      Logical replication protocol uses a single byte character to identify a
      message type in logical replication protocol. The code uses string
      literals for the same. Use Enum so that
      
      1. All the string literals used can be found at a single place. This
      makes it easy to add more types without the risk of conflicts.
      
      2. It's easy to locate the code handling a given message type.
      
      3. When used with switch statements, it is easy to identify the missing
      cases using -Wswitch.
      
      Author: Ashutosh Bapat
      Reviewed-by: Kyotaro Horiguchi, Andres Freund, Peter Smith and Amit Kapila
      Discussion: https://postgr.es/m/CAExHW5uPzQ7L0oAd_ENyvaiYMOPgkrAoJpE+ZY5-obdcVT6NPg@mail.gmail.com
      644f0d7c
    • David Rowley's avatar
      Allow run-time pruning on nested Append/MergeAppend nodes · a929e17e
      David Rowley authored
      Previously we only tagged on the required information to allow the
      executor to perform run-time partition pruning for Append/MergeAppend
      nodes belonging to base relations.  It was thought that nested
      Append/MergeAppend nodes were just about always pulled up into the
      top-level Append/MergeAppend and that making the run-time pruning info for
      any sub Append/MergeAppend nodes was a waste of time.  However, that was
      likely badly thought through.
      
      Some examples of cases we're unable to pullup nested Append/MergeAppends
      are: 1) Parallel Append nodes with a mix of parallel and non-parallel
      paths into a Parallel Append.  2) When planning an ordered Append scan a
      sub-partition which is unordered may require a nested MergeAppend path to
      ensure sub-partitions don't mix up the order of tuples being fed into the
      top-level Append.
      
      Unfortunately, it was not just as simple as removing the lines in
      createplan.c which were purposefully not building the run-time pruning
      info for anything but RELOPT_BASEREL relations.  The code in
      add_paths_to_append_rel() was far too sloppy about which partitioned_rels
      it included for the Append/MergeAppend paths.  The original code there
      would always assume accumulate_append_subpath() would pull each sub-Append
      and sub-MergeAppend path into the top-level path.  While it does not
      appear that there were any actual bugs caused by having the additional
      partitioned table RT indexes recorded, what it did mean is that later in
      planning, when we built the run-time pruning info that we wasted effort
      and built PartitionedRelPruneInfos for partitioned tables that we had no
      subpaths for the executor to run-time prune.
      
      Here we tighten that up so that partitioned_rels only ever contains the RT
      index for partitioned tables which actually have subpaths in the given
      Append/MergeAppend.  We can now Assert that every PartitionedRelPruneInfo
      has a non-empty present_parts.  That should allow us to catch any weird
      corner cases that have been missed.
      
      In passing, it seems there is no longer a good reason to have the
      AppendPath and MergeAppendPath's partitioned_rel fields a List of IntList.
      We can simply have a List of Relids instead.  This is more compact in
      memory and faster to add new members to.  We still know which is the root
      level partition as these always have a lower relid than their children.
      Previously this field was used for more things, but run-time partition
      pruning now remains the only user of it and it has no need for a List of
      IntLists.
      
      Here we also get rid of the RelOptInfo partitioned_child_rels field. This
      is what was previously used to (sometimes incorrectly) set the
      Append/MergeAppend path's partitioned_rels field.  That was the only usage
      of that field, so we can happily just remove it.
      
      I also couldn't resist changing some nearby code to make use of the newly
      added for_each_from macro so we can skip the first element in the list
      without checking if the current item was the first one on each
      iteration.
      
      A bug report from Andreas Kretschmer prompted all this work, however,
      after some consideration, I'm not personally classing this as a bug fix.
      So no backpatch.  In Andreas' test case, it just wasn't that clear that
      there was a nested Append since the top-level Append just had a single
      sub-path which was pulled up a level, per 8edd0e79.
      
      Author: David Rowley
      Reviewed-by: Amit Langote
      Discussion: https://postgr.es/m/flat/CAApHDvqSchs%2BubdybcfFaSPB%2B%2BEA7kqMaoqajtP0GtZvzOOR3g%40mail.gmail.com
      a929e17e
  4. 01 Nov, 2020 2 commits
    • Tom Lane's avatar
      Fix two issues in TOAST decompression. · dfc79773
      Tom Lane authored
      pglz_maximum_compressed_size() potentially underestimated the amount
      of compressed data required to produce N bytes of decompressed data;
      this is a fault in commit 11a078cf.
      
      Separately from that, pglz_decompress() failed to protect itself
      against corrupt compressed data, particularly off == 0 in a match
      tag.  Commit c60e520f turned such a situation into an infinite loop,
      where before it'd just have resulted in garbage output.
      
      The combination of these two bugs seems like it may explain bug #16694
      from Tom Vijlbrief, though it's impossible to be quite sure without
      direct inspection of the failing session.  (One needs to assume that
      the pglz_maximum_compressed_size() bug caused us to fail to fetch the
      second byte of a match tag, and what happened to be there instead was
      a zero.  The reported infinite loop is hard to explain without off == 0,
      though.)
      
      Aside from fixing the bugs, rewrite associated comments for more
      clarity.
      
      Back-patch to v13 where both these commits landed.
      
      Discussion: https://postgr.es/m/16694-f107871e499ec114@postgresql.org
      dfc79773
    • Tom Lane's avatar
      Avoid null pointer dereference if error result lacks SQLSTATE. · 7f423503
      Tom Lane authored
      Although error results received from the backend should always have
      a SQLSTATE field, ones generated by libpq won't, making this code
      vulnerable to a crash after, say, untimely loss of connection.
      Noted by Coverity.
      
      Oversight in commit 403a3d91.  Back-patch to 9.5, as that was.
      7f423503