1. 31 Jan, 2021 4 commits
  2. 30 Jan, 2021 7 commits
    • Peter Eisentraut's avatar
      Add primary keys and unique constraints to system catalogs · dfb75e47
      Peter Eisentraut authored
      For those system catalogs that have a unique indexes, make a primary
      key and unique constraint, using ALTER TABLE ... PRIMARY KEY/UNIQUE
      USING INDEX.
      
      This can be helpful for GUI tools that look for a primary key, and it
      might in the future allow declaring foreign keys, for making schema
      diagrams.
      
      The constraint creation statements are automatically created by
      genbki.pl from DECLARE_UNIQUE_INDEX directives.  To specify which one
      of the available unique indexes is the primary key, use the new
      directive DECLARE_UNIQUE_INDEX_PKEY instead.  By convention, we
      usually make a catalog's OID column its primary key, if it has one.
      Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Discussion: https://www.postgresql.org/message-id/flat/dc5f44d9-5ec1-a596-0251-dadadcdede98@2ndquadrant.com
      dfb75e47
    • Peter Eisentraut's avatar
      doc: Clarify status of SELECT INTO on reference page · 65330622
      Peter Eisentraut authored
      The documentation as well as source code comments weren't entirely
      clear whether SELECT INTO was truly deprecated (thus in theory
      destined to be removed eventually), or just a less recommended
      variant.  After discussion, it appears that other implementations also
      use SELECT INTO in direct SQL in a way similar to PostgreSQL, so it
      seems worth keeping it for compatibility.  Update the language in the
      documentation to that effect.
      
      Discussion: https://www.postgresql.org/message-id/flat/96dc0df3-e13a-a85d-d045-d6e2c85218da%40enterprisedb.com
      65330622
    • Peter Eisentraut's avatar
      Allow GRANTED BY clause in normal GRANT and REVOKE statements · 6aaaa76b
      Peter Eisentraut authored
      The SQL standard allows a GRANTED BY clause on GRANT and
      REVOKE (privilege) statements that can specify CURRENT_USER or
      CURRENT_ROLE.  In PostgreSQL, both of these are the default behavior.
      Since we already have all the parsing support for this for the
      GRANT (role) statement, we might as well add basic support for this
      for the privilege variant as well.  This allows us to check off SQL
      feature T332.  In the future, perhaps more interesting things could be
      done with this, too.
      Reviewed-by: default avatarSimon Riggs <simon@2ndquadrant.com>
      Discussion: https://www.postgresql.org/message-id/flat/f2feac44-b4c5-f38f-3699-2851d6a76dc9@2ndquadrant.com
      6aaaa76b
    • Noah Misch's avatar
      Revive "snapshot too old" with wal_level=minimal and SET TABLESPACE. · 7da83415
      Noah Misch authored
      Given a permanent relation rewritten in the current transaction, the
      old_snapshot_threshold mechanism assumed the relation had never been
      subject to early pruning.  Hence, a query could fail to report "snapshot
      too old" when the rewrite followed an early truncation.  ALTER TABLE SET
      TABLESPACE is probably the only rewrite mechanism capable of exposing
      this bug.  REINDEX sets indcheckxmin, avoiding the problem.  CLUSTER has
      zeroed page LSNs since before old_snapshot_threshold existed, so
      old_snapshot_threshold has never cooperated with it.  ALTER TABLE
      ... SET DATA TYPE makes the table look empty to every past snapshot,
      which is strictly worse.  Back-patch to v13, where commit
      c6b92041 broke this.
      
      Kyotaro Horiguchi and Noah Misch
      
      Discussion: https://postgr.es/m/20210113.160705.2225256954956139776.horikyota.ntt@gmail.com
      7da83415
    • Noah Misch's avatar
      Fix error with CREATE PUBLICATION, wal_level=minimal, and new tables. · 360bd232
      Noah Misch authored
      CREATE PUBLICATION has failed spuriously when applied to a permanent
      relation created or rewritten in the current transaction.  Make the same
      change to another site having the same semantic intent; the second
      instance has no user-visible consequences.  Back-patch to v13, where
      commit c6b92041 broke this.
      
      Kyotaro Horiguchi
      
      Discussion: https://postgr.es/m/20210113.160705.2225256954956139776.horikyota.ntt@gmail.com
      360bd232
    • Noah Misch's avatar
      Fix CREATE INDEX CONCURRENTLY for simultaneous prepared transactions. · 8a54e12a
      Noah Misch authored
      In a cluster having used CREATE INDEX CONCURRENTLY while having enabled
      prepared transactions, queries that use the resulting index can silently
      fail to find rows.  Fix this for future CREATE INDEX CONCURRENTLY by
      making it wait for prepared transactions like it waits for ordinary
      transactions.  This expands the VirtualTransactionId structure domain to
      admit prepared transactions.  It may be necessary to reindex to recover
      from past occurrences.  Back-patch to 9.5 (all supported versions).
      
      Andrey Borodin, reviewed (in earlier versions) by Tom Lane and Michael
      Paquier.
      
      Discussion: https://postgr.es/m/2E712143-97F7-4890-B470-4A35142ABC82@yandex-team.ru
      8a54e12a
    • Fujii Masao's avatar
      postgres_fdw: Fix tests for CLOBBER_CACHE_ALWAYS. · f77717b2
      Fujii Masao authored
      The regression tests added in commits 708d165d and 411ae649 caused
      buildfarm failures when  CLOBBER_CACHE_ALWAYS was enabled.
      This commit stabilizes those tests.
      
      The foreign server connections established by postgres_fdw behaves
      differently depending on whether CLOBBER_CACHE_ALWAYS is enabled or not.
      If it's not enabled, those connections are cached. On the other hand,
      if it's enabled, when the connections are established outside transaction
      block, they are not cached (i.e., they are immediately closed at the end of
      query that established them). So the subsequent postgres_fdw_get_connections()
      cannot list those connections and postgres_fdw_disconnect() cannot close them
      (because they are already closed).
      
      When the connections are established inside transaction block, they are
      cached whether CLOBBER_CACHE_ALWAYS was enabled or not. But if it's enabled,
      they are immediately marked as invalid, otherwise not. This causes the
      subsequent postgres_fdw_get_connections() to return different result in
      "valid" column depending on whether CLOBBER_CACHE_ALWAYS was enabled or not.
      
      This commit prevents the above differences of behavior from
      affecting the regression tests.
      
      Per buildfarm failure on trilobite.
      
      Original patch by Bharath Rupireddy. I (Fujii Masao) extracted
      the regression test fix from that and revised it a bit.
      
      Reported-by: Tom Lane
      Author: Bharath Rupireddy
      Reviewed-by: Fujii Masao
      Discussion: https://postgr.es/m/2688508.1611865371@sss.pgh.pa.us
      f77717b2
  3. 29 Jan, 2021 7 commits
  4. 28 Jan, 2021 11 commits
    • Tom Lane's avatar
      Silence another gcc 11 warning. · 1046dbed
      Tom Lane authored
      Per buildfarm and local experimentation, bleeding-edge gcc isn't
      convinced that the MemSet in reorder_function_arguments() is safe.
      Shut it up by adding an explicit check that pronargs isn't negative,
      and by changing MemSet to memset.  (It appears that either change is
      enough to quiet the warning at -O2, but let's do both to be sure.)
      1046dbed
    • Alvaro Herrera's avatar
      Remove bogus restriction from BEFORE UPDATE triggers · 6f5c8a8e
      Alvaro Herrera authored
      In trying to protect the user from inconsistent behavior, commit
      487e9861 "Enable BEFORE row-level triggers for partitioned tables"
      tried to prevent BEFORE UPDATE FOR EACH ROW triggers from moving the row
      from one partition to another.  However, it turns out that the
      restriction is wrong in two ways: first, it fails spuriously, preventing
      valid situations from working, as in bug #16794; and second, they don't
      protect from any misbehavior, because tuple routing would cope anyway.
      
      Fix by removing that restriction.
      
      We keep the same restriction on BEFORE INSERT FOR EACH ROW triggers,
      though.  It is valid and useful there.  In the future we could remove it
      by having tuple reroute work for inserts as it does for updates.
      
      Backpatch to 13.
      
      Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
      Reported-by: default avatarPhillip Menke <pg@pmenke.de>
      Discussion: https://postgr.es/m/16794-350a655580fbb9ae@postgresql.org
      6f5c8a8e
    • Tom Lane's avatar
      Fix hash partition pruning with asymmetric partition sets. · 1d9351a8
      Tom Lane authored
      perform_pruning_combine_step() was not taught about the number of
      partition indexes used in hash partitioning; more embarrassingly,
      get_matching_hash_bounds() also had it wrong.  These errors are masked
      in the common case where all the partitions have the same modulus
      and no partition is missing.  However, with missing or unequal-size
      partitions, we could erroneously prune some partitions that need
      to be scanned, leading to silently wrong query answers.
      
      While a minimal-footprint fix for this could be to export
      get_partition_bound_num_indexes and make the incorrect functions use it,
      I'm of the opinion that that function should never have existed in the
      first place.  It's not reasonable data structure design that
      PartitionBoundInfoData lacks any explicit record of the length of
      its indexes[] array.  Perhaps that was all right when it could always
      be assumed equal to ndatums, but something should have been done about
      it as soon as that stopped being true.  Putting in an explicit
      "nindexes" field makes both partition_bounds_equal() and
      partition_bounds_copy() simpler, safer, and faster than before,
      and removes explicit knowledge of the number-of-partition-indexes
      rules from some other places too.
      
      This change also makes get_hash_partition_greatest_modulus obsolete.
      I left that in place in case any external code uses it, but no core
      code does anymore.
      
      Per bug #16840 from Michał Albrycht.  Back-patch to v11 where the
      hash partitioning code came in.  (In the back branches, add the new
      field at the end of PartitionBoundInfoData to minimize ABI risks.)
      
      Discussion: https://postgr.es/m/16840-571a22976f829ad4@postgresql.org
      1d9351a8
    • Tom Lane's avatar
      Make ecpg's rjulmdy() and rmdyjul() agree with their declarations. · 1b242f42
      Tom Lane authored
      We had "short *mdy" in the extern declarations, but "short mdy[3]"
      in the actual function definitions.  Per C99 these are equivalent,
      but recent versions of gcc have started to issue warnings about
      the inconsistency.  Clean it up before the warnings get any more
      widespread.
      
      Back-patch, in case anyone wants to build older PG versions with
      bleeding-edge compilers.
      
      Discussion: https://postgr.es/m/2401575.1611764534@sss.pgh.pa.us
      1b242f42
    • Alvaro Herrera's avatar
      pgbench: Remove dead code · 6819b904
      Alvaro Herrera authored
      doConnect() never returns connections in state CONNECTION_BAD, so
      checking for that is pointless.  Remove the code that does.
      
      This code has been dead since ba708ea3, 20 years ago.
      
      Discussion: https://postgr.es/m/20210126195224.GA20361@alvherre.pgsqlReviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      6819b904
    • Peter Eisentraut's avatar
      Remove gratuitous uses of deprecated SELECT INTO · b034ef9b
      Peter Eisentraut authored
      CREATE TABLE AS has been preferred over SELECT INTO (outside of ecpg
      and PL/pgSQL) for a long time.  There were still a few uses of SELECT
      INTO in tests and documentation, some old, some more recent.  This
      changes them to CREATE TABLE AS.  Some occurrences in the tests remain
      where they are specifically testing SELECT INTO parsing or similar.
      
      Discussion: https://www.postgresql.org/message-id/flat/96dc0df3-e13a-a85d-d045-d6e2c85218da%40enterprisedb.com
      b034ef9b
    • Heikki Linnakangas's avatar
      Add direct conversion routines between EUC_TW and Big5. · 6c557607
      Heikki Linnakangas authored
      Conversions between EUC_TW and Big5 were previously implemented by
      converting the whole input to MIC first, and then from MIC to the target
      encoding. Implement functions to convert directly between the two.
      
      The reason to do this now is that I'm working on a patch that will change
      the conversion function signature so that if the input is invalid, we
      convert as much as we can and return the number of bytes successfully
      converted. That's not possible if we use an intermediary format, because
      if an error happens in the intermediary -> final conversion, we lose track
      of the location of the invalid character in the original input. Avoiding
      the intermediate step makes the conversions faster, too.
      
      Reviewed-by: John Naylor
      Discussion: https://www.postgresql.org/message-id/b9e3167f-f84b-7aa4-5738-be578a4db924%40iki.fi
      6c557607
    • Heikki Linnakangas's avatar
      Add mbverifystr() functions specific to each encoding. · b80e1063
      Heikki Linnakangas authored
      This makes pg_verify_mbstr() function faster, by allowing more efficient
      encoding-specific implementations. All the implementations included in
      this commit are pretty naive, they just call the same encoding-specific
      verifychar functions that were used previously, but that already gives a
      performance boost because the tight character-at-a-time loop is simpler.
      
      Reviewed-by: John Naylor
      Discussion: https://www.postgresql.org/message-id/e7861509-3960-538a-9025-b75a61188e01@iki.fi
      b80e1063
    • Andrew Gierth's avatar
      Don't add bailout adjustment for non-strict deserialize calls. · a3367aa3
      Andrew Gierth authored
      When building aggregate expression steps, strict checks need a bailout
      jump for when a null value is encountered, so there is a list of steps
      that require later adjustment. Adding entries to that list for steps
      that aren't actually strict would be harmless, except that there is an
      Assert which catches them. This leads to spurious errors on asserts
      builds, for data sets that trigger parallel aggregation of an
      aggregate with a non-strict deserialization function (no such
      aggregates exist in the core system).
      
      Repair by not adding the adjustment entry when it's not needed.
      
      Backpatch back to 11 where the code was introduced.
      
      Per a report from Darafei (Komzpa) of the PostGIS project; analysis
      and patch by me.
      
      Discussion: https://postgr.es/m/87mty7peb3.fsf@news-spur.riddles.org.uk
      a3367aa3
    • Michael Paquier's avatar
      Fix crash of pg_stat_statements_info() without library loaded · bca96dda
      Michael Paquier authored
      Other code paths are already protected against this case, and _PG_init()
      warns about that in pg_stat_statements.c.  While on it, I have checked
      the other extensions of the tree but did not notice any holes.
      
      Oversight in 9fbc3f31.
      
      Author: Jaime Casanova
      Reviewed-by: Julien Rouhaud
      Discussion: https://postgr.es/m/CAJKUy5gF4=_=qhJ1VX_tSGFfjKHb9BvzhRYWSApJD=Bfwp2SBw@mail.gmail.com
      bca96dda
    • Michael Paquier's avatar
      Refactor SQL functions of SHA-2 in cryptohashfuncs.c · f854c69a
      Michael Paquier authored
      The same code pattern was repeated four times when compiling a SHA-2
      hash.  This refactoring has the advantage to issue a compilation warning
      if a new value is added to pg_cryptohash_type, so as anybody doing an
      addition in this area would need to consider if support for a new SQL
      function is needed or not.
      
      Author: Sehrope Sarkuni, Michael Paquier
      Discussion: https://postgr.es/m/YA7DvLRn2xnTgsMc@paquier.xyz
      f854c69a
  5. 27 Jan, 2021 8 commits
    • Peter Geoghegan's avatar
      Reduce the default value of vacuum_cost_page_miss. · e19594c5
      Peter Geoghegan authored
      When commit f425b605 introduced cost based vacuum delays back in 2004,
      the defaults reflected then-current trends in hardware, as well as
      certain historical limitations in PostgreSQL.  There have been enormous
      improvements in both areas since that time.  The cost limit GUC defaults
      finally became much more representative of current trends following
      commit cbccac37, which decreased autovacuum_vacuum_cost_delay's default
      by 10x for PostgreSQL 12 (it went from 20ms to only 2ms).
      
      The relative costs have shifted too.  This should also be accounted for
      by the defaults.  More specifically, the relative importance of avoiding
      dirtying pages within VACUUM has greatly increased, primarily due to
      main memory capacity scaling and trends in flash storage.  Within
      Postgres itself, improvements like sequential access during index
      vacuuming (at least in nbtree and GiST indexes) have also been
      contributing factors.
      
      To reflect all this, decrease the default of vacuum_cost_page_miss to 2.
      Since the default of vacuum_cost_page_dirty remains 20, dirtying a page
      is now considered 10x "costlier" than a page miss by default.
      
      Author: Peter Geoghegan <pg@bowt.ie>
      Discussion: https://postgr.es/m/CAH2-WzmLPFnkWT8xMjmcsm7YS3+_Qi3iRWAb2+_Bc8UhVyHfuA@mail.gmail.com
      e19594c5
    • Robert Haas's avatar
      In TrimCLOG(), don't reset XactCtl->shared->latest_page_number. · 69059d3b
      Robert Haas authored
      Since the CLOG page number is not recorded directly in the checkpoint
      record, we have to use ShmemVariableCache->nextXid to figure out the
      latest CLOG page number at the start of recovery. However, as recovery
      progresses, replay of CLOG/EXTEND records will update our notion of
      the latest page number, and we should rely on that being accurate
      rather than recomputing the value based on an updated notion of
      nextXid. ShmemVariableCache->nextXid is only an approximation
      during recovery anyway, whereas CLOG/EXTEND records are an
      authoritative representation of how the SLRU has been updated.
      
      Commit 0fcc2dec makes this
      simplification possible, as before that change clog_redo() might
      have injected a bogus value here, and we'd want to get rid of
      that before entering normal running.
      
      Patch by me, reviewed by Heikki Linnakangas.
      
      Discussion: http://postgr.es/m/CA+TgmoZYig9+AQodhF5sRXuKkJ=RgFDugLr3XX_dz_F-p=TwTg@mail.gmail.com
      69059d3b
    • Robert Haas's avatar
      In clog_redo(), don't set XactCtl->shared->latest_page_number. · 0fcc2dec
      Robert Haas authored
      The comment is no longer accurate, and hasn't been entirely accurate
      since Hot Standby was introduced. The original idea here was that
      StartupCLOG() wouldn't be called until the end of recovery and
      therefore this value would be uninitialized when this code is reached,
      but Hot Standby made that true only when hot_standby=off, and commit
      1f113abd means that this value is now
      always initialized before replay even starts.
      
      The original purpose of this code was to bypass the sanity check
      in SimpleLruTruncate(), which will no longer occur: now, if something
      is wrong, that sanity check might trip during recovery. That's
      probably a good thing, because in the current code base
      latest_page_number should always be initialized and therefore we
      expect that the sanity check should pass. If it doesn't, something
      has gone wrong, and complaining about it is appropriate.
      
      Patch by me, reviewed by Heikki Linnakangas.
      
      Discussion: http://postgr.es/m/CA+TgmoZYig9+AQodhF5sRXuKkJ=RgFDugLr3XX_dz_F-p=TwTg@mail.gmail.com
      0fcc2dec
    • Tom Lane's avatar
      Doc: improve documentation for UNNEST(). · 662affcf
      Tom Lane authored
      Per a user question, spell out that UNNEST() returns array elements
      in storage order; also provide an example to clarify the behavior for
      multi-dimensional arrays.
      
      While here, also clarify the SELECT reference page's description of
      WITH ORDINALITY.  These details were already given in 7.2.1.4, but
      a reference page should not omit details.
      
      Back-patch to v13; there's not room in the table in older versions.
      
      Discussion: https://postgr.es/m/FF1FB31F-0507-4F18-9559-2DE6E07E3B43@gmail.com
      662affcf
    • Robert Haas's avatar
      Move StartupCLOG() calls to just after we initialize ShmemVariableCache. · 1f113abd
      Robert Haas authored
      Previously, the hot_standby=off code path did this at end of recovery,
      while the hot_standby=on code path did it at the beginning of recovery.
      It's better to do this in only one place because (a) it's simpler,
      (b) StartupCLOG() is trivial so trying to postpone the work isn't
      useful, and (c) this will make it possible to simplify some other
      logic.
      
      Patch by me, reviewed by Heikki Linnakangas.
      
      Discussion: http://postgr.es/m/CA+TgmoZYig9+AQodhF5sRXuKkJ=RgFDugLr3XX_dz_F-p=TwTg@mail.gmail.com
      1f113abd
    • Peter Geoghegan's avatar
      Fix GiST index deletion assert issue. · e42b3c3b
      Peter Geoghegan authored
      Avoid calling heap_index_delete_tuples() with an empty deltids array to
      avoid an assertion failure.
      
      This issue was arguably an oversight in commit b5f58cf2, though the
      failing assert itself was added by my recent commit d168b666.  No
      backpatch, though, since the oversight is harmless in the back branches.
      
      Author: Peter Geoghegan <pg@bowt.ie>
      Reported-By: default avatarJaime Casanova <jcasanov@systemguards.com.ec>
      Discussion: https://postgr.es/m/CAJKUy5jscES84n3puE=sYngyF+zpb4wv8UMtuLnLPv5z=6yyNw@mail.gmail.com
      e42b3c3b
    • Michael Paquier's avatar
      doc: Remove reference to views for TRUNCATE privilege · 32bef758
      Michael Paquier authored
      The page about privilege rights mentioned that TRUNCATE could be applied
      to views or even other relation types.  This is confusing as this
      command can be used only on tables and on partitioned tables.
      
      Oversight in afc4a78a.
      
      Reported-by: Harisai Hari
      Reviewed-by: Laurenz Albe
      Discussion: https://postgr.es/m/161157636877.14625.15340884663716426087@wrigleys.postgresql.org
      Backpatch-through: 12
      32bef758
    • Michael Paquier's avatar
      Refactor code in tablecmds.c to check and process tablespace moves · 4c9c359d
      Michael Paquier authored
      Two code paths of tablecmds.c (for relations with storage and without
      storage) use the same logic to check if the move of a relation to a
      new tablespace is allowed or not and to update pg_class.reltablespace
      and pg_class.relfilenode.
      
      A potential TABLESPACE clause for REINDEX, CLUSTER and VACUUM FULL needs
      similar checks to make sure that nothing is moved around in illegal ways
      (no mapped relations, shared relations only in pg_global, no move of
      temp tables owned by other backends).
      
      This reorganizes the existing code of ALTER TABLE so as all this logic
      is controlled by two new routines that can be reused for the other
      commands able to move relations across tablespaces, limiting the number
      of code paths in need of the same protections.  This also removes some
      code that was duplicated for tables with and without storage for ALTER
      TABLE.
      
      Author: Alexey Kondratov, Michael Paquier
      Discussion: https://postgr.es/m/YA+9mAMWYLXJMVPL@paquier.xyz
      4c9c359d
  6. 26 Jan, 2021 3 commits
    • Tom Lane's avatar
      Rethink recently-added SPI interfaces. · d5a83d79
      Tom Lane authored
      SPI_execute_with_receiver and SPI_cursor_parse_open_with_paramlist are
      new in v14 (cf. commit 2f48ede0).  Before they can get out the door,
      let's change their APIs to follow the practice recently established by
      SPI_prepare_extended etc: shove all optional arguments into a struct
      that callers are supposed to pre-zero.  The hope is to allow future
      addition of more options without either API breakage or a continuing
      proliferation of new SPI entry points.  With that in mind, choose
      slightly more generic names for them: SPI_execute_extended and
      SPI_cursor_parse_open respectively.
      
      Discussion: https://postgr.es/m/CAFj8pRCLPdDAETvR7Po7gC5y_ibkn_-bOzbeJb39WHms01194Q@mail.gmail.com
      d5a83d79
    • Tom Lane's avatar
      Suppress compiler warnings from commit ee895a65. · 7292fd8f
      Tom Lane authored
      For obscure reasons, some buildfarm members are now generating
      complaints about plpgsql_call_handler's "retval" variable possibly
      being used uninitialized.  It seems no less safe than it was before
      that commit, but these complaints are (mostly?) new.  I trust that
      initializing the variable where it's declared will be enough to
      shut that up.
      
      I also notice that some compilers are warning about setjmp clobber
      of the same variable, which is maybe a bit more defensible.  Mark
      it volatile to silence that.
      
      Also, rearrange the logic to give procedure_resowner a single
      point of initialization, in hopes of silencing some setjmp-clobber
      warnings about that.  (Marking it volatile would serve too, but
      its sibling variables are depending on single assignment, so let's
      stick with that method.)
      
      Discussion: https://postgr.es/m/E1l4F1z-0000cN-Lx@gemulon.postgresql.org
      7292fd8f
    • Tom Lane's avatar
      Code review for psql's helpSQL() function. · f76a8500
      Tom Lane authored
      The loops to identify word boundaries could access past the end of
      the input string.  Likely that would never result in an actual
      crash, but it makes valgrind unhappy.
      
      The logic to try different numbers of words didn't work when the
      input has two words but we only have a match to the first, eg
      "\h with select".  (We must "continue" the pass loop, not "break".)
      
      The logic to compute nl_count was bizarrely managed, and in at
      least two code paths could end up calling PageOutput with
      nl_count = 0, resulting in failing to paginate output that should
      have been fed to the pager.  Also, in v12 and up, the nl_count
      calculation hadn't been updated to account for the addition of a URL.
      
      The PQExpBuffer holding the command syntax details wasn't freed,
      resulting in a session-lifespan memory leak.
      
      While here, improve some comments, choose a more descriptive name
      for a variable, fix inconsistent datatype choice for another variable.
      
      Per bug #16837 from Alexander Lakhin.  This code is very old,
      so back-patch to all supported branches.
      
      Kyotaro Horiguchi and Tom Lane
      
      Discussion: https://postgr.es/m/16837-479bcd56040c71b3@postgresql.org
      f76a8500