1. 10 Dec, 2019 3 commits
    • Alvaro Herrera's avatar
      Add backend-only appendStringInfoStringQuoted · 6cafde1b
      Alvaro Herrera authored
      This provides a mechanism to emit literal values in informative
      messages, such as query parameters.  The new code is more complex than
      what it replaces, primarily because it wants to be more efficient.
      It also has the (currently unused) additional optional capability of
      specifying a maximum size to print.
      
      The new function lives out of common/stringinfo.c so that frontend users
      of that file need not pull in unnecessary multibyte-encoding support
      code.
      
      Author: Álvaro Herrera and Alexey Bashtanov, after a suggestion from Andres Freund
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de
      6cafde1b
    • Tom Lane's avatar
      In pg_ctl, work around ERROR_SHARING_VIOLATION on the postmaster log file. · 0da33c76
      Tom Lane authored
      On Windows, we use CMD.EXE to redirect the postmaster's stdout/stderr
      into a log file.  CMD.EXE will open that file with non-sharing-friendly
      parameters, and the file will remain open for a short time after the
      postmaster has removed postmaster.pid.  This can result in an
      ERROR_SHARING_VIOLATION failure if we attempt to start a new postmaster
      immediately with the same log file (e.g. during "pg_ctl restart").
      This seems to explain intermittent buildfarm failures we've been seeing
      on Windows machines.
      
      To fix, just open and close the log file using our own pgwin32_open(),
      which will wait if necessary to avoid the failure.  (Perhaps someday
      we should stop using CMD.EXE, but that would be a far more complex
      patch, and it doesn't seem worth the trouble ... yet.)
      
      Back-patch to v12.  This only solves the problem when frontend fopen()
      is redirected to pgwin32_fopen(), which has only been true since commit
      0ba06e0b.  Hence, no point in back-patching further, unless we care
      to back-patch that change too.
      
      Diagnosis and patch by Alexander Lakhin (bug #16154).
      
      Discussion: https://postgr.es/m/16154-1ccf0b537b24d5e0@postgresql.org
      0da33c76
    • Etsuro Fujita's avatar
      Fix handling of multiple AFTER ROW triggers on a foreign table. · 5a20b021
      Etsuro Fujita authored
      AfterTriggerExecute() retrieves a fresh tuple or pair of tuples from a
      tuplestore and then stores the tuple(s) in the passed-in slot(s) if
      AFTER_TRIGGER_FDW_FETCH, while it uses the most-recently-retrieved
      tuple(s) stored in the slot(s) if AFTER_TRIGGER_FDW_REUSE.  This was
      done correctly before 12, but commit ff11e7f4 broke it by mistakenly
      clearing the tuple(s) stored in the slot(s) in that function, leading to
      an assertion failure as reported in bug #16139 from Alexander Lakhin.
      
      Also, fix some other issues with the aforementioned commit in passing:
      
      * For tg_newslot, which is a slot added to the TriggerData struct by the
        commit to store new updated tuples, it didn't ensure the slot was NULL
        if there was no such tuple.
      * The commit failed to update the documentation about the trigger
        interface.
      
      Author: Etsuro Fujita
      Backpatch-through: 12
      Discussion: https://postgr.es/m/16139-94f9ccf0db6119ec%40postgresql.org
      5a20b021
  2. 09 Dec, 2019 3 commits
    • Tom Lane's avatar
      Fix race condition in our Windows signal emulation. · 28e6a2fd
      Tom Lane authored
      pg_signal_dispatch_thread() responded to the client (signal sender)
      and disconnected the pipe before actually setting the shared variables
      that make the signal visible to the backend process's main thread.
      In the worst case, it seems, effective delivery of the signal could be
      postponed for as long as the machine has any other work to do.
      
      To fix, just move the pg_queue_signal() call so that we do it before
      responding to the client.  This essentially makes pgkill() synchronous,
      which is a stronger guarantee than we have on Unix.  That may be
      overkill, but on the other hand we have not seen comparable timing bugs
      on any Unix platform.
      
      While at it, add some comments to this sadly underdocumented code.
      
      Problem diagnosis and fix by Amit Kapila; I just added the comments.
      
      Back-patch to all supported versions, as it appears that this can cause
      visible NOTIFY timing oddities on all of them, and there might be
      other misbehavior due to slow delivery of other signals.
      
      Discussion: https://postgr.es/m/32745.1575303812@sss.pgh.pa.us
      28e6a2fd
    • Tom Lane's avatar
      Improve isolationtester's timeout management. · 99351a8b
      Tom Lane authored
      isolationtester.c had a hard-wired limit of 3 minutes per test step.
      It now emerges that this isn't quite enough for some of the slowest
      buildfarm animals.  This isn't the first time we've had to raise
      this limit (cf. 1db439ad), so let's make it configurable.  This
      patch raises the default to 5 minutes, and introduces an environment
      variable PGISOLATIONTIMEOUT that can be set if more time is needed,
      following the precedent of PGCTLTIMEOUT.
      
      Also, modify isolationtester so that when the timeout is hit,
      it explicitly reports having sent a cancel.  This makes the regression
      failure log considerably more intelligible.  (In the worst case, a
      timed-out test might actually be reported as "passing" without this
      extra output, so arguably this is a bug fix in itself.)
      
      In passing, update the README file, which had apparently not gotten
      touched when we added "make check" support here.
      
      Back-patch to 9.6; older versions don't have comparable timeout logic.
      
      Discussion: https://postgr.es/m/22964.1575842935@sss.pgh.pa.us
      99351a8b
    • Amit Kapila's avatar
      Fix typos in miscinit.c. · 2d0fdfac
      Amit Kapila authored
      Commit f13ea95f moved the description of postmaster.pid file contents
      from miscadmin.h to pidfile.h, but missed to update the comments in
      miscinit.c.
      
      Author: Hadi Moshayedi
      Reviewed-by: Amit Kapila
      Backpatch-through: 10
      Discussion: https://postgr.es/m/CAK=1=WpYEM9x3LGkaxgXaxeYQjnkdW8XLsxrYRTE2Gq-H83FMw@mail.gmail.com
      2d0fdfac
  3. 08 Dec, 2019 2 commits
  4. 07 Dec, 2019 1 commit
  5. 06 Dec, 2019 6 commits
    • Tom Lane's avatar
      Improve test coverage of ruleutils.c. · 830d1c73
      Tom Lane authored
      While fooling around with the EXPLAIN improvements I've been working
      on, I noticed that there were some large gaps in our test coverage
      of ruleutils.c, according to the code coverage report.  This commit
      just adds a few test cases to improve coverage of:
      get_name_for_var_field()
      get_update_query_targetlist_def()
      isSimpleNode()
      get_sublink_expr()
      830d1c73
    • Jeff Davis's avatar
      Fix comments in execGrouping.c · 30d47723
      Jeff Davis authored
      Commit 5dfc1981 missed updating some comments.
      
      Also, fix a comment typo found in passing.
      
      Author: Jeff Davis
      Discussion: https://postgr.es/m/9723131d247b919f94699152647fa87ee0bc02c2.camel%40j-davis.com
      30d47723
    • Tom Lane's avatar
      Disallow non-default collation in ADD PRIMARY KEY/UNIQUE USING INDEX. · fbbf6809
      Tom Lane authored
      When creating a uniqueness constraint using a pre-existing index,
      we have always required that the index have the same properties you'd
      get if you just let a new index get built.  However, when collations
      were added, we forgot to add the index's collation to that check.
      
      It's hard to trip over this without intentionally trying to break it:
      you'd have to explicitly specify a different collation in CREATE
      INDEX, then convert it to a pkey or unique constraint.  Still, if you
      did that, pg_dump would emit a script that fails to reproduce the
      index's collation.  The main practical problem is that after a
      pg_upgrade the index would be corrupt, because its actual physical
      order wouldn't match what pg_index says.  A more theoretical issue,
      which is new as of v12, is that if you create the index with a
      nondeterministic collation then it wouldn't be enforcing the normal
      notion of uniqueness, causing the constraint to mean something
      different from a normally-created constraint.
      
      To fix, just add collation to the conditions checked for index
      acceptability in ADD PRIMARY KEY/UNIQUE USING INDEX.  We won't try
      to clean up after anybody who's already created such a situation;
      it seems improbable enough to not be worth the effort involved.
      (If you do get into trouble, a REINDEX should be enough to fix it.)
      
      In principle this is a long-standing bug, but I chose not to
      back-patch --- the odds of causing trouble seem about as great
      as the odds of preventing it, and both risks are very low anyway.
      
      Per report from Alexey Bashtanov, though this is not his preferred
      fix.
      
      Discussion: https://postgr.es/m/b05ce36a-cefb-ca5e-b386-a400535b1c0b@imap.cc
      fbbf6809
    • Michael Paquier's avatar
      Fix handling of OpenSSL's SSL_clear_options · 7d0bcb04
      Michael Paquier authored
      This function is supported down to OpenSSL 0.9.8, which is the oldest
      version supported since 593d4e47 (from Postgres 10 onwards), and is used
      since e3bdb2d9 (from 11 onwards).  It is defined as a macro from OpenSSL
      0.9.8 to 1.0.2, and as a function in 1.1.0 and newer versions.  However,
      the configure check present is only adapted for functions.  So, even if
      the code would be able to compile, configure fails to detect the macro,
      causing it to be ignored when compiling the code with OpenSSL from 0.9.8
      to 1.0.2.
      
      The code needs a configure check as per a364dfa4, which has fixed a
      compilation issue with a past version of LibreSSL in NetBSD 5.1.  On
      HEAD, just remove the configure check as the last release of NetBSD 5 is
      from 2014 (and we have no more buildfarm members for it).  In 11 and 12,
      improve the configure logic so as both macros and functions are
      correctly detected.  This makes NetBSD 5 still work on already-released
      branches, but not for 13 onwards.
      
      The patch for HEAD is from me, and Daniel has written the version to use
      for the back-branches.
      
      Author: Michael Paquier, Daniel Gustaffson
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
      Discussion: https://postgr.es/m/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se
      Backpatch-through: 11
      7d0bcb04
    • Michael Paquier's avatar
      Improve some comments in pg_upgrade.c · 690c8802
      Michael Paquier authored
      When restoring database schemas on a new cluster, database "template1"
      is processed first, followed by all other databases in parallel,
      including "postgres".  Both "postgres" and "template1" have some extra
      handling to propagate each one's properties, but comments were confusing
      regarding which one is processed where.
      
      Author: Julien Rouhaud
      Reviewed-by: Daniel Gustafsson
      Discussion: https://postgr.es/m/CAOBaU_a2iviTG7FE10yO_gcW+zQCHNFhRA_NDiktf3UR65BHdw@mail.gmail.com
      690c8802
    • Michael Paquier's avatar
      Remove configure check for OpenSSL's SSL_get_current_compression() · 28f4bba6
      Michael Paquier authored
      This function has been added in OpenSSL 0.9.8, which is the oldest
      version supported on HEAD, so checking for it at configure time is
      useless.  Both the frontend and backend code did not even bother to use
      it.
      
      Reported-by: Daniel Gustafsson
      Author: Michael Paquier
      Reviewed-by: Daniel Gustafsson, Tom Lane
      Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
      Discussion: https://postgr.es/m/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se
      28f4bba6
  6. 05 Dec, 2019 2 commits
  7. 04 Dec, 2019 6 commits
  8. 03 Dec, 2019 7 commits
    • Tomas Vondra's avatar
      Ensure maxlen is at leat 1 in dict_int · b5273943
      Tomas Vondra authored
      The dict_int text search dictionary template accepts maxlen parameter,
      which is then used to cap the length of input strings. The value was
      not properly checked, and the code simply does
      
          txt[d->maxlen] = '\0';
      
      to insert a terminator, leading to segfaults with negative values.
      
      This commit simply rejects values less than 1. The issue was there since
      dct_int was introduced in 9.3, so backpatch all the way back to 9.4
      which is the oldest supported version.
      
      Reported-by: cili
      Discussion: https://postgr.es/m/16144-a36a5bef7657047d@postgresql.org
      Backpatch-through: 9.4
      b5273943
    • Tom Lane's avatar
      Further sync postgres_fdw's "Relations" output with the rest of EXPLAIN. · bf39b3af
      Tom Lane authored
      EXPLAIN generally only adds schema qualifications to table names when
      VERBOSE is specified.  In postgres_fdw's "Relations" output, table
      names were always so qualified, but that was an implementation
      restriction: in the original coding, we didn't have access to the
      verbose flag at the time the string was generated.  After the code
      rearrangement of commit 4526951d, we do have that info available
      at the right time, so make this output follow the normal rule.
      
      Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us
      bf39b3af
    • Michael Paquier's avatar
      Fix thinkos from commit 9989d37d · 68ab9829
      Michael Paquier authored
      Error messages referring to incorrect WAL segment names could have been
      generated for a fsync() failure or when creating a new segment at the
      end of recovery.
      68ab9829
    • Peter Eisentraut's avatar
      Fix alter_system_table test · 88d45ac7
      Peter Eisentraut authored
      Add workaround for disabling ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
      warning for the test that tries to create a tablespace with a reserved
      name.
      
      Discussion: https://www.postgresql.org/message-id/flat/E1iacW7-0003h6-6U%40gemulon.postgresql.org
      88d45ac7
    • Michael Paquier's avatar
      Remove XLogFileNameP() from the tree · 9989d37d
      Michael Paquier authored
      XLogFileNameP() is a wrapper routine able to build a palloc'd string for
      a WAL segment name, which is used for error string generation.  There
      were several code paths where it gets called in a critical section,
      where memory allocation is not allowed.  This results in triggering
      an assertion failure instead of generating the wanted error message.
      
      Another, more annoying, problem is that if the allocation to generate
      the WAL segment name fails on OOM, then the failure would be escalated
      to a PANIC.
      
      This removes the routine and all its callers are replaced with a logic
      using a fixed-size buffer.  This way, all the existing mistakes are
      fixed and future ones are prevented.
      
      Author: Masahiko Sawada
      Reviewed-by: Michael Paquier, Álvaro Herrera
      Discussion: https://postgr.es/m/CA+fd4k5gC9H4uoWMLg9K_QfNrnkkdEw+-AFveob9YX7z8JnKTA@mail.gmail.com
      9989d37d
    • Michael Paquier's avatar
      Fix failures with TAP tests of pg_ctl on Windows · e5532f19
      Michael Paquier authored
      On Windows, all the hosts spawned by the TAP tests bind to 127.0.0.1.
      Hence, if there is a port conflict, starting a cluster would immediately
      fail.  One of the test scripts of pg_ctl initializes a node without
      PostgresNode.pm, using the default port 5432.  This could cause
      unexpected startup failures in the tests if an independent server was up
      and running on the same host (the reverse is also possible, though more
      unlikely).  Fix this issue by assigning properly a free port to the node
      configured, in the same range used as for the other nodes part of the
      tests.
      
      Author: Michael Paquier
      Reviewed-by: Andrew Dunstan
      Discussion: https://postgr.es/m/20191202031444.GC1696@paquier.xyz
      Backpatch-through: 11
      e5532f19
    • Tom Lane's avatar
      Fix EXPLAIN's column alias output for mismatched child tables. · 55a1954d
      Tom Lane authored
      If an inheritance/partitioning parent table is assigned some column
      alias names in the query, EXPLAIN mapped those aliases onto the
      child tables' columns by physical position, resulting in bogus output
      if a child table's columns aren't one-for-one with the parent's.
      
      To fix, make expand_single_inheritance_child() generate a correctly
      re-mapped column alias list, rather than just copying the parent
      RTE's alias node.  (We have to fill the alias field, not just
      adjust the eref field, because ruleutils.c will ignore eref in
      favor of looking at the real column names.)
      
      This means that child tables will now always have alias fields in
      plan rtables, where before they might not have.  That results in
      a rather substantial set of regression test output changes:
      EXPLAIN will now always show child tables with aliases that match
      the parent table (usually with "_N" appended for uniqueness).
      But that seems like a net positive for understandability, since
      the parent alias corresponds to something that actually appeared
      in the original query, while the child table names didn't.
      (Note that this does not change anything for cases where an explicit
      table alias was written in the query for the parent table; it
      just makes cases without such aliases behave similarly to that.)
      Hence, while we could avoid these subsidiary changes if we made
      inherit.c more complicated, we choose not to.
      
      Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us
      55a1954d
  9. 02 Dec, 2019 4 commits
    • Tom Lane's avatar
      Add a reverse-translation column number array to struct AppendRelInfo. · ce76c0ba
      Tom Lane authored
      This provides for cheaper mapping of child columns back to parent
      columns.  The one existing use-case in examine_simple_variable()
      would hardly justify this by itself; but an upcoming bug fix will
      make use of this array in a mainstream code path, and it seems
      likely that we'll find other uses for it as we continue to build
      out the partitioning infrastructure.
      
      Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us
      ce76c0ba
    • Tom Lane's avatar
      Make postgres_fdw's "Relations" output agree with the rest of EXPLAIN. · 4526951d
      Tom Lane authored
      The relation aliases shown in the "Relations" line for a foreign scan
      didn't always agree with those used in the rest of EXPLAIN's output.
      The regression test result changes appearing here provide examples.
      
      It's really impossible for postgres_fdw to duplicate EXPLAIN's alias
      assignment logic during postgresGetForeignRelSize(), because of the
      de-duplication that EXPLAIN does on a global basis --- and anyway,
      trying to duplicate that would be unmaintainable.  Instead, just put
      numeric rangetable indexes into the string, and convert those to
      table names/aliases in postgresExplainForeignScan, which does have
      access to the results of ruleutils.c's alias assignment logic.
      Aside from being more reliable, this shifts some work from planning
      to EXPLAIN, which is a good tradeoff for performance.  (I also
      changed from using StringInfo to using psprintf, which makes the
      code slightly simpler and reduces its memory consumption.)
      
      A kluge required by this solution is that we have to reverse-engineer
      the rtoffset applied by setrefs.c.  If that logic ever fails
      (presumably because the member tables of a join got offset by
      different amounts), we'll need some more cooperation with setrefs.c
      to keep things straight.  But for now, there's no need for that.
      
      Arguably this is a back-patchable bug fix, but since this is a mostly
      cosmetic issue and there have been no field complaints, I'll refrain
      for now.
      
      Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us
      4526951d
    • Michael Paquier's avatar
      Add query cancellation capabilities in pgbench init phase · 1d468b9a
      Michael Paquier authored
      This can be useful to stop data generation happening on the server for
      long-running queries caused by large scale factors.  This cannot happen
      by default as data is generated on the client, but it is possible to
      control the initialization steps of pgbench to do that.
      
      Reported-by: Fujii Masao
      Author: Fabien Coelho
      Discussion: https://postgr.es/m/alpine.DEB.2.21.1910311939430.27369@lancre
      Discussion: https://postgr.es/m/CAHGQGwHWEyTXxZh46qgFY8a2bDF_EYeUdp3+_Hy=qLZSzwVPKg@mail.gmail.com
      1d468b9a
    • Michael Paquier's avatar
      Refactor query cancellation code into src/fe_utils/ · a4fd3aa7
      Michael Paquier authored
      Originally, this code was duplicated in src/bin/psql/ and
      src/bin/scripts/, but it can be useful for other frontend applications,
      like pgbench.  This refactoring offers the possibility to setup a custom
      callback which would get called in the signal handler for SIGINT or when
      the interruption console events happen on Windows.
      
      Author: Fabien Coelho, with contributions from Michael Paquier
      Reviewed-by: Álvaro Herrera, Ibrar Ahmed
      Discussion: https://postgr.es/m/alpine.DEB.2.21.1910311939430.27369@lancre
      a4fd3aa7
  10. 01 Dec, 2019 2 commits
    • Andrew Dunstan's avatar
      Add dummy versions of new SSL functions for non-SSL builds · c01ac6dc
      Andrew Dunstan authored
      This rectifies an oversight in commit 4dc63552, which caused certain
      builds to fail, especially on Windows.
      c01ac6dc
    • Tom Lane's avatar
      Fix misbehavior with expression indexes on ON COMMIT DELETE ROWS tables. · c35b714c
      Tom Lane authored
      We implement ON COMMIT DELETE ROWS by truncating tables marked that
      way, which requires also truncating/rebuilding their indexes.  But
      RelationTruncateIndexes asks the relcache for up-to-date copies of any
      index expressions, which may cause execution of eval_const_expressions
      on them, which can result in actual execution of subexpressions.
      This is a bad thing to have happening during ON COMMIT.  Manuel Rigger
      reported that use of a SQL function resulted in crashes due to
      expectations that ActiveSnapshot would be set, which it isn't.
      The most obvious fix perhaps would be to push a snapshot during
      PreCommit_on_commit_actions, but I think that would just open the door
      to more problems: CommitTransaction explicitly expects that no
      user-defined code can be running at this point.
      
      Fortunately, since we know that no tuples exist to be indexed, there
      seems no need to use the real index expressions or predicates during
      RelationTruncateIndexes.  We can set up dummy index expressions
      instead (we do need something that will expose the right data type,
      as there are places that build index tupdescs based on this), and
      just ignore predicates and exclusion constraints.
      
      In a green field it'd likely be better to reimplement ON COMMIT DELETE
      ROWS using the same "init fork" infrastructure used for unlogged
      relations.  That seems impractical without catalog changes though,
      and even without that it'd be too big a change to back-patch.
      So for now do it like this.
      
      Per private report from Manuel Rigger.  This has been broken forever,
      so back-patch to all supported branches.
      c35b714c
  11. 30 Nov, 2019 3 commits
    • Andrew Dunstan's avatar
      libq support for sslpassword connection param, DER format keys · 4dc63552
      Andrew Dunstan authored
      This patch providies for support for password protected SSL client
      keys in libpq, and for DER format keys, both encrypted and unencrypted.
      There is a new connection parameter sslpassword, which is supplied to
      the OpenSSL libraries via a callback function. The callback function can
      also be set by an application by calling PQgetSSLKeyPassHook(). There is
      also a function to retreive the connection setting, PQsslpassword().
      
      Craig Ringer and Andrew Dunstan
      
      Reviewed by: Greg Nancarrow
      
      Discussion: https://postgr.es/m/f7ee88ed-95c4-95c1-d4bf-7b415363ab62@2ndQuadrant.com
      4dc63552
    • Tomas Vondra's avatar
      Fix off-by-one error in PGTYPEStimestamp_fmt_asc · 3ff660bb
      Tomas Vondra authored
      When using %b or %B patterns to format a date, the code was simply using
      tm_mon as an index into array of month names. But that is wrong, because
      tm_mon is 1-based, while array indexes are 0-based. The result is we
      either use name of the next month, or a segfault (for December).
      
      Fix by subtracting 1 from tm_mon for both patterns, and add a regression
      test triggering the issue. Backpatch to all supported versions (the bug
      is there far longer, since at least 2003).
      
      Reported-by: Paul Spencer
      Backpatch-through: 9.4
      Discussion: https://postgr.es/m/16143-0d861eb8688d3fef%40postgresql.org
      3ff660bb
    • Amit Kapila's avatar
      Revert commits 290acac9 and 8a7e9e9d. · 98a9b37b
      Amit Kapila authored
      This commit revert the commits to add a test case that tests the 'force'
      option when there is an active backend connected to the database being
      dropped.
      
      This feature internally sends SIGTERM to all the backends connected to the
      database being dropped and then the same is reported to the client.  We
      found that on Windows, the client end of the socket is not able to read
      the data once we close the socket in the server which leads to loss of
      error message which is not what we expect.  We also observed  similar
      behavior in other cases like pg_terminate_backend(),
      pg_ctl kill TERM <pid>.  There are probably a few others like that.  The
      fix for this requires further study.
      
      Discussion: https://postgr.es/m/E1iaD8h-0004us-K9@gemulon.postgresql.org
      98a9b37b
  12. 29 Nov, 2019 1 commit