1. 28 Oct, 2020 6 commits
    • Tom Lane's avatar
      Don't use custom OID symbols in pg_proc.dat. · 36b93121
      Tom Lane authored
      We have a perfectly good convention for OID macros for built-in functions
      already, so making custom symbols is just introducing unnecessary
      deviation from the convention.  Remove the one case that had snuck in,
      and add an error check in genbki.pl to discourage future instances.
      
      Although this touches pg_proc.dat, there's no need for a catversion
      bump since the actual catalog data isn't changed.
      
      John Naylor
      
      Discussion: https://postgr.es/m/CAFBsxsHpCbjfoddNGpnnnY5pHwckWfiYkMYSF74PmP1su0+ZOw@mail.gmail.com
      36b93121
    • Tom Lane's avatar
      Fix foreign-key selectivity estimation in the presence of constants. · ad1c36b0
      Tom Lane authored
      get_foreign_key_join_selectivity() looks for join clauses that equate
      the two sides of the FK constraint.  However, if we have a query like
      "WHERE fktab.a = pktab.a and fktab.a = 1", it won't find any such join
      clause, because equivclass.c replaces the given clauses with "fktab.a
      = 1 and pktab.a = 1", which can be enforced at the scan level, leaving
      nothing to be done for column "a" at the join level.
      
      We can fix that expectation without much trouble, but then a new problem
      arises: applying the foreign-key-based selectivity rule produces a
      rowcount underestimate, because we're effectively double-counting the
      selectivity of the "fktab.a = 1" clause.  So we have to cancel that
      selectivity out of the estimate.
      
      To fix, refactor process_implied_equality() so that it can pass back the
      new RestrictInfo to its callers in equivclass.c, allowing the generated
      "fktab.a = 1" clause to be saved in the EquivalenceClass's ec_derives
      list.  Then it's not much trouble to dig out the relevant RestrictInfo
      when we need to adjust an FK selectivity estimate.  (While at it, we
      can also remove the expensive use of initialize_mergeclause_eclasses()
      to set up the new RestrictInfo's left_ec and right_ec pointers.
      The equivclass.c code can set those basically for free.)
      
      This seems like clearly a bug fix, but I'm hesitant to back-patch it,
      first because there's some API/ABI risk for extensions and second because
      we're usually loath to destabilize plan choices in stable branches.
      
      Per report from Sigrid Ehrenreich.
      
      Discussion: https://postgr.es/m/1019549.1603770457@sss.pgh.pa.us
      Discussion: https://postgr.es/m/AM6PR02MB5287A0ADD936C1FA80973E72AB190@AM6PR02MB5287.eurprd02.prod.outlook.com
      ad1c36b0
    • Michael Paquier's avatar
      Use correct GetDatum() in pg_relation_check_pages() · ce7f772c
      Michael Paquier authored
      UInt32GetDatum() was getting used, while the result needs
      Int64GetDatum().  Oversight in f2b88396.
      
      Per buildfarm member florican.
      
      Discussion: https://postgr.es/m/1226629.1603859189@sss.pgh.pa.us
      ce7f772c
    • Michael Paquier's avatar
      Add pg_relation_check_pages() to check on-disk pages of a relation · f2b88396
      Michael Paquier authored
      This makes use of CheckBuffer() introduced in c780a7a9, adding a SQL
      wrapper able to do checks for all the pages of a relation.  By default,
      all the fork types of a relation are checked, and it is possible to
      check only a given relation fork.  Note that if the relation given in
      input has no physical storage or is temporary, then no errors are
      generated, allowing full-database checks when coupled with a simple scan
      of pg_class for example.  This is not limited to clusters with data
      checksums enabled, as clusters without data checksums can still apply
      checks on pages using the page headers or for the case of a page full of
      zeros.
      
      This function returns a set of tuples consisting of:
      - The physical file where a broken page has been detected (without the
      segment number as that can be AM-dependent, which can be guessed from
      the block number for heap).  A relative path from PGPATH is used.
      - The block number of the broken page.
      
      By default, only superusers have an access to this function but
      execution rights can be granted to other users.
      
      The feature introduced here is still minimal, and more improvements
      could be done, like:
      - Addition of a start and end block number to run checks on a range
      of blocks, which would apply only if one fork type is checked.
      - Addition of some progress reporting.
      - Throttling, with configuration parameters in function input or
      potentially some cost-based GUCs.
      
      Regression tests are added for positive cases in the main regression
      test suite, and TAP tests are added for cases involving the emulation of
      page corruptions.
      
      Bump catalog version.
      
      Author: Julien Rouhaud, Michael Paquier
      Reviewed-by: Masahiko Sawada, Justin Pryzby
      Discussion: https://postgr.es/m/CAOBaU_aVvMjQn=ge5qPiJOPMmOj5=ii3st5Q0Y+WuLML5sR17w@mail.gmail.com
      f2b88396
    • Michael Paquier's avatar
      Add CheckBuffer() to check on-disk pages without shared buffer loading · c780a7a9
      Michael Paquier authored
      CheckBuffer() is designed to be a concurrent-safe function able to run
      sanity checks on a relation page without loading it into the shared
      buffers.  The operation is done using a lock on the partition involved
      in the shared buffer mapping hashtable and an I/O lock for the buffer
      itself, preventing the risk of false positives due to any concurrent
      activity.
      
      The primary use of this function is the detection of on-disk corruptions
      for relation pages.  If a page is found in shared buffers, the on-disk
      page is checked if not dirty (a follow-up checkpoint would flush a valid
      version of the page if dirty anyway), as it could be possible that a
      page was present for a long time in shared buffers with its on-disk
      version corrupted.  Such a scenario could lead to a corrupted cluster if
      a host is plugged off for example.  If the page is not found in shared
      buffers, its on-disk state is checked.  PageIsVerifiedExtended() is used
      to apply the same sanity checks as when a page gets loaded into shared
      buffers.
      
      This function will be used by an upcoming patch able to check the state
      of on-disk relation pages using a SQL function.
      
      Author: Julien Rouhaud, Michael Paquier
      Reviewed-by:  Masahiko Sawada
      Discussion: https://postgr.es/m/CAOBaU_aVvMjQn=ge5qPiJOPMmOj5=ii3st5Q0Y+WuLML5sR17w@mail.gmail.com
      c780a7a9
    • Amit Kapila's avatar
  2. 27 Oct, 2020 11 commits
  3. 26 Oct, 2020 4 commits
  4. 25 Oct, 2020 2 commits
    • Tom Lane's avatar
      Fix corner case for a BEFORE ROW UPDATE trigger returning OLD. · ba9f18ab
      Tom Lane authored
      If the old row has any "missing" attributes that are supposed to
      be retrieved from an associated tuple descriptor, the wrong things
      happened because the trigger result is shoved directly into an
      executor slot that lacks the missing-attribute data.  Notably,
      CHECK-constraint verification would incorrectly see those columns
      as NULL, and so would RETURNING-list evaluation.
      
      Band-aid around this by forcibly expanding the tuple before passing
      it to the trigger function.  (IMO it was a fundamental misdesign to
      put the missing-attribute data into tuple constraints, which so
      much of the system considers to be optional.  But we're probably
      stuck with that now, and will have to continue to apply band-aids
      as we find other places with similar issues.)
      
      Back-patch to v12.  v11 would also have the issue, except that
      commit 920311ab1 already applied a similar band-aid.  That forced
      expansion in more cases than seem really necessary, though, so
      this isn't a directly equivalent fix.
      
      Amit Langote, with some cosmetic changes by me
      
      Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org
      ba9f18ab
    • David Rowley's avatar
      Fix incorrect parameter name in a function header comment · e83c9f91
      David Rowley authored
      Author: Zhijie Hou
      Discussion: https://postgr.es/m/14cd74ea00204cc8a7ea5d738ac82cd1@G08CNEXMBPEKD05.g08.fujitsu.local
      Backpatch-through: 12, where the mistake was introduced
      e83c9f91
  5. 24 Oct, 2020 3 commits
  6. 23 Oct, 2020 10 commits
  7. 22 Oct, 2020 4 commits
    • Tom Lane's avatar
      Add documentation and tests for quote marks in ECPG literal queries. · c16a1bbc
      Tom Lane authored
      ECPG's PREPARE ... FROM and EXECUTE IMMEDIATE can optionally take
      the target query as a simple literal, rather than the more usual
      string-variable reference.  This was previously documented as
      being a C string literal, but that's a lie in one critical respect:
      you can't write a data double quote as \" in such literals.  That's
      because the lexer is in SQL mode at this point, so it'll parse
      double-quoted strings as SQL identifiers, within which backslash
      is not special, so \" ends the literal.
      
      I looked into making this work as documented, but getting the lexer
      to switch behaviors at just the right point is somewhere between
      very difficult and impossible.  It's not really worth the trouble,
      because these cases are next to useless: if you have a fixed SQL
      statement to execute or prepare, you might as well write it as
      a direct EXEC SQL, saving the messiness of converting it into
      a string literal and gaining the opportunity for compile-time
      SQL syntax checking.
      
      Instead, let's just document (and test) the workaround of writing
      a double quote as an octal escape (\042) in such cases.
      
      There's no code behavioral change here, so in principle this could
      be back-patched, but it's such a niche case I doubt it's worth
      the trouble.
      
      Per report from 1250kv.
      
      Discussion: https://postgr.es/m/673825.1603223178@sss.pgh.pa.us
      c16a1bbc
    • Tom Lane's avatar
      Avoid premature de-doubling of quote marks in ECPG strings. · 3dfb1942
      Tom Lane authored
      If you write the literal 'abc''def' in an EXEC SQL command, that will
      come out the other end as 'abc'def', triggering a syntax error in the
      backend.  Likewise, "abc""def" is reduced to "abc"def" which is wrong
      syntax for a quoted identifier.
      
      The cause is that the lexer thinks it should emit just one quote
      mark, whereas what it really should do is keep the string as-is.
      
      Add some docs and test cases, too.
      
      Although this seems clearly a bug, I fear users wouldn't appreciate
      changing it in minor releases.  Some may well be working around it
      by applying an extra doubling of affected quotes, as for example
      sql/dyntest.pgc has been doing.
      
      Per investigation of a report from 1250kv, although this isn't
      exactly what he/she was on about.
      
      Discussion: https://postgr.es/m/673825.1603223178@sss.pgh.pa.us
      3dfb1942
    • Robert Haas's avatar
      Try to avoid a compiler warning about using fxid uninitialized. · 8bb0c977
      Robert Haas authored
      Mark Dilger, with a couple of stray semicolons removed by me.
      
      Discussion: http://postgr.es/m/2A7DA1A8-C4AA-43DF-A985-3CA52F4DC775@enterprisedb.com
      8bb0c977
    • Tom Lane's avatar
      Clean up some unpleasant behaviors in psql's \connect command. · 94929f1c
      Tom Lane authored
      The check for whether to complain about not having an old connection
      to get parameters from was seriously out of date: it had not been
      rethought when we invented connstrings, nor when we invented the
      -reuse-previous option.  Replace it with a check that throws an
      error if reuse-previous is active and we lack an old connection to
      reuse.  While that doesn't move the goalposts very far in terms of
      easing reconnection after a server crash, at least it's consistent.
      
      If the user specifies a connstring plus additional parameters
      (which is invalid per the documentation), the extra parameters were
      silently ignored.  That seems like it could be really confusing,
      so let's throw a syntax error instead.
      
      Teach the connstring code path to re-use the old connection's password
      in the same cases as the old-style-syntax code path would, ie if we
      are reusing parameters and the values of username, host/hostaddr, and
      port are not being changed.  Document this behavior, too, since it was
      unmentioned before.  Also simplify the implementation a bit, giving
      rise to two new and useful properties: if there's a "password=xxx" in
      the connstring, we'll use it not ignore it, and by default (i.e.,
      except with --no-password) we will prompt for a password if the
      re-used password or connstring password doesn't work.  The previous
      code just failed if the re-used password didn't work.
      
      Given the paucity of field complaints about these issues, I don't
      think that they rise to the level of back-patchable bug fixes,
      and in any case they might represent undesirable behavior changes
      in minor releases.  So no back-patch.
      
      Discussion: https://postgr.es/m/235210.1603321144@sss.pgh.pa.us
      94929f1c