1. 23 Sep, 2020 3 commits
    • Tom Lane's avatar
      Improve error cursor positions for problems with partition bounds. · 6b2c4e59
      Tom Lane authored
      We failed to pass down the query string to check_new_partition_bound,
      so that its attempts to provide error cursor positions were for naught;
      one must have the query string to get parser_errposition to do anything.
      Adjust its API to require a ParseState to be passed down.
      
      Also, improve the logic inside check_new_partition_bound so that the
      cursor points at the partition bound for the specific column causing
      the issue, when one can be identified.
      
      That part is also for naught if we can't determine the query position of
      the column with the problem.  Improve transformPartitionBoundValue so
      that it makes sure that const-simplified partition expressions will be
      properly labeled with positions.  In passing, skip calling evaluate_expr
      if the value is already a Const, which is surely the most common case.
      
      Alexandra Wang, Ashwin Agrawal, Amit Langote; reviewed by Ashutosh Bapat
      
      Discussion: https://postgr.es/m/CACiyaSopZoqssfMzgHk6fAkp01cL6vnqBdmTw2C5_KJaFR_aMg@mail.gmail.com
      Discussion: https://postgr.es/m/CAJV4CdrZ5mKuaEsRSbLf2URQ3h6iMtKD=hik8MaF5WwdmC9uZw@mail.gmail.com
      6b2c4e59
    • Tom Lane's avatar
      Avoid possible dangling-pointer access in tsearch_readline_callback. · 3ea7e955
      Tom Lane authored
      tsearch_readline() saves the string pointer it returns to the caller
      for possible use in the associated error context callback.  However,
      the caller will usually pfree that string sometime before it next
      calls tsearch_readline(), so that there is a window where an ereport
      will try to print an already-freed string.
      
      The built-in users of tsearch_readline() happen to all do that pfree
      at the bottoms of their loops, so that the window is effectively
      empty for them.  However, this is not documented as a requirement,
      and contrib/dict_xsyn doesn't do it like that, so it seems likely
      that third-party dictionaries might have live bugs here.
      
      The practical consequences of this seem pretty limited in any case,
      since production builds wouldn't clobber the freed string immediately,
      besides which you'd not expect syntax errors in dictionary files
      being used in production.  Still, it's clearly a bug waiting to bite
      somebody.
      
      Fix by pstrdup'ing the string to be saved for the error callback,
      and then pfree'ing it next time through.  It's been like this for
      a long time, so back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/48A4FA71-524E-41B9-953A-FD04EF36E2E7@yesql.se
      3ea7e955
    • Thomas Munro's avatar
      Allow WaitLatch() to be used without a latch. · 733fa9aa
      Thomas Munro authored
      Due to flaws in commit 3347c982, using WaitLatch() without
      WL_LATCH_SET could cause an assertion failure or crash.  Repair.
      
      While here, also add a check that the latch we're switching to belongs
      to this backend, when changing from one latch to another.
      
      Discussion: https://postgr.es/m/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com
      733fa9aa
  2. 22 Sep, 2020 5 commits
  3. 21 Sep, 2020 4 commits
  4. 20 Sep, 2020 2 commits
  5. 19 Sep, 2020 3 commits
  6. 18 Sep, 2020 6 commits
  7. 17 Sep, 2020 12 commits
    • Tom Lane's avatar
      Remove support for postfix (right-unary) operators. · 1ed6b895
      Tom Lane authored
      This feature has been a thorn in our sides for a long time, causing
      many grammatical ambiguity problems.  It doesn't seem worth the
      pain to continue to support it, so remove it.
      
      There are some follow-on improvements we can make in the grammar,
      but this commit only removes the bare minimum number of productions,
      plus assorted backend support code.
      
      Note that pg_dump and psql continue to have full support, since
      they may be used against older servers.  However, pg_dump warns
      about postfix operators.  There is also a check in pg_upgrade.
      
      Documentation-wise, I (tgl) largely removed the "left unary"
      terminology in favor of saying "prefix operator", which is
      a more standard and IMO less confusing term.
      
      I included a catversion bump, although no initial catalog data
      changes here, to mark the boundary at which oprkind = 'r'
      stopped being valid in pg_operator.
      
      Mark Dilger, based on work by myself and Robert Haas;
      review by John Naylor
      
      Discussion: https://postgr.es/m/38ca86db-42ab-9b48-2902-337a0d6b8311@2ndquadrant.com
      1ed6b895
    • Tom Lane's avatar
      Remove factorial operators, leaving only the factorial() function. · 76f412ab
      Tom Lane authored
      The "!" operator is our only built-in postfix operator.  Remove it,
      on the way to removal of grammar support for postfix operators.
      
      There is also a "!!" prefix operator, but since it's been marked
      deprecated for most of its existence, we might as well remove it too.
      
      Also zap the SQL alias function numeric_fac(), which seems to have
      equally little reason to live.
      
      Mark Dilger, based on work by myself and Robert Haas;
      review by John Naylor
      
      Discussion: https://postgr.es/m/38ca86db-42ab-9b48-2902-337a0d6b8311@2ndquadrant.com
      76f412ab
    • Tom Lane's avatar
      Further improve pgindent's list of file exclusions. · 74d4608f
      Tom Lane authored
      I despair of people keeping the README file's notes in sync with
      the actual exclusion list, so move the notes into the exclusion file.
      Adjust the pgindent script to explicitly ignore comments in the file,
      just in case (though it's hard to believe any would match filenames).
      
      Extend the list so that it doesn't process any files we'd not wish it to
      even in a fully-built-out development directory.  (There are still a
      couple of derived files that it wants to reformat, but fixing the
      generator scripts for those seems like fit material for a separate patch.)
      
      Discussion: https://postgr.es/m/79ed5348-be7a-b647-dd40-742207186a22@2ndquadrant.com
      74d4608f
    • Tom Lane's avatar
      Improve common/logging.c's support for multiple verbosity levels. · 99175141
      Tom Lane authored
      Instead of hard-wiring specific verbosity levels into the option
      processing of client applications, invent pg_logging_increase_verbosity()
      and encourage clients to implement --verbose by calling that.  Then,
      the common convention that more -v's gets you more verbosity just works.
      
      In particular, this allows resurrection of the debug-grade messages that
      have long existed in pg_dump and its siblings.  They were unreachable
      before this commit due to lack of a way to select PG_LOG_DEBUG logging
      level.  (It appears that they may have been unreachable for some time
      before common/logging.c was introduced, too, so I'm not specifically
      blaming cc8d4151 for the oversight.  One reason for thinking that is
      that it's now apparent that _allocAH()'s message needs a null-pointer
      guard.  Testing might have failed to reveal that before 96bf88d5.)
      
      Discussion: https://postgr.es/m/1173106.1600116625@sss.pgh.pa.us
      99175141
    • Amit Kapila's avatar
      Update parallel BTree scan state when the scan keys can't be satisfied. · b7f2dd95
      Amit Kapila authored
      For parallel btree scan to work for array of scan keys, it should reach
      BTPARALLEL_DONE state once for every distinct combination of array keys.
      This is required to ensure that the parallel workers don't try to seize
      blocks at the same time for different scan keys. We missed to update this
      state when we discovered that the scan keys can't be satisfied.
      
      Author: James Hunter
      Reviewed-by: Amit Kapila
      Tested-by: Justin Pryzby
      Backpatch-through: 10, where it was introduced
      Discussion: https://postgr.es/m/4248CABC-25E3-4809-B4D0-128E1BAABC3C@amazon.com
      b7f2dd95
    • Peter Eisentraut's avatar
      Allow CURRENT_ROLE where CURRENT_USER is accepted · 45b98057
      Peter Eisentraut authored
      In the particular case of GRANTED BY, this is specified in the SQL
      standard.  Since in PostgreSQL, CURRENT_ROLE is equivalent to
      CURRENT_USER, and CURRENT_USER is already supported here, adding
      CURRENT_ROLE is trivial.  The other cases are PostgreSQL extensions,
      but for the same reason it also makes sense there.
      Reviewed-by: default avatarVik Fearing <vik@postgresfriends.org>
      Reviewed-by: default avatarAsif Rehman <asifr.rehman@gmail.com>
      Reviewed-by: default avatarAlvaro Herrera <alvherre@2ndquadrant.com>
      Discussion: https://www.postgresql.org/message-id/flat/f2feac44-b4c5-f38f-3699-2851d6a76dc9%402ndquadrant.com
      45b98057
    • Heikki Linnakangas's avatar
      Add support for building GiST index by sorting. · 16fa9b2b
      Heikki Linnakangas authored
      This adds a new optional support function to the GiST access method:
      sortsupport. If it is defined, the GiST index is built by sorting all data
      to the order defined by the sortsupport's comparator function, and packing
      the tuples in that order to GiST pages. This is similar to how B-tree
      index build works, and is much faster than inserting the tuples one by
      one. The resulting index is smaller too, because the pages are packed more
      tightly, upto 'fillfactor'. The normal build method works by splitting
      pages, which tends to lead to more wasted space.
      
      The quality of the resulting index depends on how good the opclass-defined
      sort order is. A good order preserves locality of the input data.
      
      As the first user of this facility, add 'sortsupport' function to the
      point_ops opclass. It sorts the points in Z-order (aka Morton Code), by
      interleaving the bits of the X and Y coordinates.
      
      Author: Andrey Borodin
      Reviewed-by: Pavel Borisov, Thomas Munro
      Discussion: https://www.postgresql.org/message-id/1A36620E-CAD8-4267-9067-FB31385E7C0D%40yandex-team.ru
      16fa9b2b
    • Michael Paquier's avatar
      doc: Apply more consistently <productname> markup for OpenSSL · 089da3c4
      Michael Paquier authored
      OpenSSL was quoted in inconsistent ways in many places of the docs,
      sometimes with <application>, <productname> or just nothing.
      
      Author: Daniel Gustafsson
      Discussion: https://postgr.es/m/DA91E5F0-5F9D-41A7-A7A6-B91CDE0F1D63@yesql.se
      089da3c4
    • Michael Paquier's avatar
      Improve tab completion of IMPORT FOREIGN SCHEMA in psql · 7307df16
      Michael Paquier authored
      It is not possible to get a list of foreign schemas as the server is not
      known, so this provides instead a list of local schemas, which is more
      useful than nothing if using a loopback server or having schema names
      matching in the local and remote servers.
      
      Author: Jeff Janes
      Reviewed-by: Tom Lane, Michael Paquier
      Discussion: https://postgr.es/m/CAMkU=1wr7Roj41q-XiJs=Uyc2xCmHhcGGy7J-peJQK-e+w=ghw@mail.gmail.com
      7307df16
    • Tom Lane's avatar
      Teach walsender to update its process title for replication commands. · babef40c
      Tom Lane authored
      Because the code path taken for SQL commands executed in a walsender
      will update the process title, we pretty much have to update the
      title for replication commands as well.  Otherwise, the title shows
      "idle" for the rest of a logical walsender's lifetime once it's
      executed any SQL command.
      
      Playing with this, I confirm that a walsender now typically spends
      most of its life reporting
      	walsender postgres [local] START_REPLICATION
      Considering this in isolation, it might be better to have it say
      	walsender postgres [local] sending replication data
      However, consistency with the other cases seems to be a stronger
      argument.
      
      In passing, remove duplicative pgstat_report_activity call.
      
      Discussion: https://postgr.es/m/880181.1600026471@sss.pgh.pa.us
      babef40c
    • Tom Lane's avatar
      Improve formatting of create_help.pl and plperl_opmask.pl output. · add10584
      Tom Lane authored
      Adjust the whitespace in the emitted files so that it matches
      what pgindent would do.  This makes the generated files look
      like they match project style, and avoids confusion if someone
      does run pgindent on the generated files.
      
      Also, add probes.h to pgindent's exclusion list, because it can
      confuse pgindent, plus there's not much point in processing it.
      
      Daniel Gustafsson, additional fixes by me
      
      Discussion: https://postgr.es/m/79ed5348-be7a-b647-dd40-742207186a22@2ndquadrant.com
      add10584
    • Alvaro Herrera's avatar
      Fix bogus completion tag usage in walsender · 07082b08
      Alvaro Herrera authored
      Since commit fd5942c1 (2012, 9.3-era), walsender has been sending
      completion tags for certain replication commands twice -- and they're
      not even consistent.  Apparently neither libpq nor JDBC have a problem
      with it, but it's not kosher.  Fix by remove the EndCommand() call in
      the common code path for them all, and inserting specific calls to
      EndReplicationCommand() specifically in those places where it's needed.
      
      EndReplicationCommand() is a new simple function to send the completion
      tag for replication commands.  Do this instead of sending a generic
      SELECT completion tag for them all, which was also pretty bogus (if
      innocuous).  While at it, change StartReplication() to use
      EndReplicationCommand() instead of pg_puttextmessage().
      
      In commit 2f966131, I failed to realize that replication commands
      are not close-enough kin of regular SQL commands, so the
      DROP_REPLICATION_SLOT tag I added is undeserved and a type pun.  Take it
      out.
      
      Backpatch to 13, where the latter commit appeared.  The duplicate tag
      has been sent since 9.3, but since nothing is broken, it doesn't seem
      worth fixing.
      
      Per complaints from Tom Lane.
      
      Discussion: https://postgr.es/m/1347966.1600195735@sss.pgh.pa.us
      07082b08
  8. 16 Sep, 2020 5 commits
    • Tom Lane's avatar
      Centralize setup of SIGQUIT handling for postmaster child processes. · 44fc6e25
      Tom Lane authored
      We decided that the policy established in commit 7634bd4f for
      the bgwriter, checkpointer, walwriter, and walreceiver processes,
      namely that they should accept SIGQUIT at all times, really ought
      to apply uniformly to all postmaster children.  Therefore, get
      rid of the duplicative and inconsistent per-process code for
      establishing that signal handler and removing SIGQUIT from BlockSig.
      Instead, make InitPostmasterChild do it.
      
      The handler set up by InitPostmasterChild is SignalHandlerForCrashExit,
      which just summarily does _exit(2).  In interactive backends, we
      almost immediately replace that with quickdie, since we would prefer
      to try to tell the client that we're dying.  However, this patch is
      changing the behavior of autovacuum (both launcher and workers), as
      well as walsenders.  Those processes formerly also used quickdie,
      but AFAICS that was just mindless copy-and-paste: they don't have
      any interactive client that's likely to benefit from being told this.
      
      The stats collector continues to be an outlier, in that it thinks
      SIGQUIT means normal exit.  That should probably be changed for
      consistency, but there's another patch set where that's being
      dealt with, so I didn't do so here.
      
      Discussion: https://postgr.es/m/644875.1599933441@sss.pgh.pa.us
      44fc6e25
    • Tom Lane's avatar
      Don't fetch partition check expression during InitResultRelInfo. · 2000b6c1
      Tom Lane authored
      Since there is only one place that actually needs the partition check
      expression, namely ExecPartitionCheck, it's better to fetch it from
      the relcache there.  In this way we will never fetch it at all if
      the query never has use for it, and we still fetch it just once when
      we do need it.
      
      The reason for taking an interest in this is that if the relcache
      doesn't already have the check expression cached, fetching it
      requires obtaining AccessShareLock on the partition root.  That
      means that operations that look like they should only touch the
      partition itself will also take a lock on the root.  In particular
      we observed that TRUNCATE on a partition may take a lock on the
      partition's root, contributing to a deadlock situation in parallel
      pg_restore.
      
      As written, this patch does have a small cost, which is that we
      are microscopically reducing efficiency for the case where a partition
      has an empty check expression.  ExecPartitionCheck will be called,
      and will go through the motions of setting up and checking an empty
      qual, where before it would not have been called at all.  We could
      avoid that by adding a separate boolean flag to track whether there
      is a partition expression to test.  However, this case only arises
      for a default partition with no siblings, which surely is not an
      interesting case in practice.  Hence adding complexity for it
      does not seem like a good trade-off.
      
      Amit Langote, per a suggestion by me
      
      Discussion: https://postgr.es/m/VI1PR03MB31670CA1BD9625C3A8C5DD05EB230@VI1PR03MB3167.eurprd03.prod.outlook.com
      2000b6c1
    • Peter Geoghegan's avatar
      Fix amcheck child check pg_upgrade bug. · aac80bfc
      Peter Geoghegan authored
      Commit d114cc53 overlooked the fact that pg_upgrade'd B-Tree indexes
      have leaf page high keys whose offset numbers do not match the one from
      the copy of the tuple one level up (the copy stored with a downlink for
      leaf page's right sibling page).  This led to false positive reports of
      corruption from bt_index_parent_check() when it was called to verify a
      pg_upgrade'd index.
      
      To fix, skip comparing the offset number on pg_upgrade'd B-Tree indexes.
      
      Author: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
      Author: Peter Geoghegan <pg@bowt.ie>
      Reported-By: default avatarAndrew Bille <andrewbille@gmail.com>
      Diagnosed-By: default avatarAnastasia Lubennikova <a.lubennikova@postgrespro.ru>
      Bug: #16619
      Discussion: https://postgr.es/m/16619-aaba10f83fdc1c3c@postgresql.org
      Backpatch: 13-, where child check was enhanced.
      aac80bfc
    • Tom Lane's avatar
      Avoid unnecessary recursion to child tables in ALTER TABLE SET NOT NULL. · e5fac1cb
      Tom Lane authored
      If a partitioned table's column is already marked NOT NULL, there is
      no need to examine its partitions, because we can rely on previous
      DDL to have enforced that the child columns are NOT NULL as well.
      (Unfortunately, the same cannot be said for traditional inheritance,
      so for now we have to restrict the optimization to partitioned tables.)
      Hence, we may skip recursing to child tables in this situation.
      
      The reason this case is worth worrying about is that when pg_dump dumps
      a partitioned table having a primary key, it will include the requisite
      NOT NULL markings in the CREATE TABLE commands, and then add the
      primary key as a separate step.  The primary key addition generates a
      SET NOT NULL as a subcommand, just to be sure.  So the situation where
      a SET NOT NULL is redundant does arise in the real world.
      
      Skipping the recursion does more than just save a few cycles: it means
      that a command such as "ALTER TABLE ONLY partition_parent ADD PRIMARY
      KEY" will take locks only on the partition parent table, not on the
      partitions.  It turns out that parallel pg_restore is effectively
      assuming that that's true, and has little choice but to do so because
      the dependencies listed for such a TOC entry don't include the
      partitions.  pg_restore could thus issue this ALTER while data restores
      on the partitions are still in progress.  Taking unnecessary locks on
      the partitions not only hurts concurrency, but can lead to actual
      deadlock failures, as reported by Domagoj Smoljanovic.
      
      (A contributing factor in the deadlock is that TRUNCATE on a child
      partition wants a non-exclusive lock on the parent.  This seems
      likewise unnecessary, but the fix for it is more invasive so we
      won't consider back-patching it.  Fortunately, getting rid of one
      of these two poor behaviors is enough to remove the deadlock.)
      
      Although support for partitioned primary keys came in with v11,
      this patch is dependent on the SET NOT NULL refactoring done by
      commit f4a3fdfb, so we can only patch back to v12.
      
      Patch by me; thanks to Alvaro Herrera and Amit Langote for review.
      
      Discussion: https://postgr.es/m/VI1PR03MB31670CA1BD9625C3A8C5DD05EB230@VI1PR03MB3167.eurprd03.prod.outlook.com
      e5fac1cb
    • Tom Lane's avatar
      Fix bogus cache-invalidation logic in logical replication worker. · 3d65b059
      Tom Lane authored
      The code recorded cache invalidation events by zeroing the "localreloid"
      field of affected cache entries.  However, it's possible for an inval
      event to occur even while we have the entry open and locked.  So an
      ill-timed inval could result in "cache lookup failed for relation 0"
      errors, if the worker's code tried to use the cleared field.  We can
      fix that by creating a separate bool field to record whether the entry
      needs to be revalidated.  (In the back branches, cram the bool into
      what had been padding space, to avoid an ABI break in the somewhat
      unlikely event that any extension is looking at this struct.)
      
      Also, rearrange the logic in logicalrep_rel_open so that it
      does the right thing in cases where table_open would fail.
      We should retry the lookup by name in that case, but we didn't.
      
      The real-world impact of this is probably small.  In the first place,
      the error conditions are very low probability, and in the second place,
      the worker would just exit and get restarted.  We only noticed because
      in a CLOBBER_CACHE_ALWAYS build, the failure can occur repeatedly,
      preventing the worker from making progress.  Nonetheless, it's clearly
      a bug, and it impedes a useful type of testing; so back-patch to v10
      where this code was introduced.
      
      Discussion: https://postgr.es/m/1032727.1600096803@sss.pgh.pa.us
      3d65b059