Skip to content

P
Postgres FD Implementation
Switch branch/tag
  1. 15 Nov, 2018 6 commits
  3. 14 Nov, 2018 8 commits
    • Tom Lane's avatar
      Make psql's "\pset format" command reject non-unique abbreviations. · eaf746a5
      Tom Lane authored 
      The previous behavior of preferring the oldest match had the advantage
of not breaking existing scripts when we add a conflicting format name;
but that behavior was undocumented and fragile (it seems just luck that
commit add9182e didn't break it).  Let's go over to the less mistake-
prone approach of complaining when there are multiple matches.

Since this is a small compatibility break, no back-patch.

Daniel Vérité

Discussion: https://postgr.es/m/cb7e1caf-3ea6-450d-af28-f524903a030c@manitou-mail.org
      eaf746a5
    • Tom Lane's avatar
      Doc: remove claim that all \pset format options are unique in 1 letter. · 51eaaafb
      Tom Lane authored 
      This hasn't been correct since 9.3 added "latex-longtable".

I left the phraseology "Unique abbreviations are allowed" alone.
It's correct as far as it goes, and we are studiously refraining
from specifying exactly what happens if you give a non-unique
abbreviation.  (The answer in the back branches is "you get a
backwards-compatible choice", and the answer in HEAD will shortly
be "you get an error", but there seems no need to mention such
details here.)

Daniel Vérité

Discussion: https://postgr.es/m/cb7e1caf-3ea6-450d-af28-f524903a030c@manitou-mail.org
      51eaaafb
    • Tom Lane's avatar
      Add a timezone-specific variant of date_trunc(). · 600b04d6
      Tom Lane authored 
      date_trunc(field, timestamptz, zone_name) performs truncation using
the named time zone as reference, rather than working in the session
time zone as is the default behavior.  It's equivalent to

date_trunc(field, timestamptz at time zone zone_name) at time zone zone_name

but it's faster, easier to type, and arguably easier to understand.

Vik Fearing and Tom Lane

Discussion: https://postgr.es/m/6249ffc4-2b22-4c1b-4e7d-7af84fedd7c6@2ndquadrant.com
      600b04d6
    • Tom Lane's avatar
      Second try at fixing numeric data passed through an ECPG SQLDA. · 06c72344
      Tom Lane authored 
      In commit ecfd5579, I removed sqlda.c's checks for ndigits != 0 on the
grounds that we should duplicate the state of the numeric value's digit
buffer even when all the digits are zeroes.  However, that still isn't
quite right, because another possible state of the digit buffer is
buf == digits == NULL (this occurs for a NaN).  As the code now stands,
it'll invoke memcpy with a NULL source address and zero bytecount,
which we know a few platforms crash on.  Hence, reinstate the no-copy
short-circuit, but make it test specifically for buf != NULL rather than
some other condition.  In hindsight, the ndigits test (added by commit
f2ae9f9c) was almost certainly meant to fix the NaN case not the
all-zeroes case as the associated thread alleged.

As before, back-patch to all supported versions.

Discussion: https://postgr.es/m/1803D792815FC24D871C00D17AE95905C71161@g01jpexmbkw24
      06c72344
    • Peter Eisentraut's avatar
      Lower lock level for renaming indexes · 1b5d797c
      Peter Eisentraut authored 
      Change lock level for renaming index (either ALTER INDEX or implicitly
via some other commands) from AccessExclusiveLock to
ShareUpdateExclusiveLock.

One reason we need a strong lock for relation renaming is that the
name change causes a rebuild of the relcache entry.  Concurrent
sessions that have the relation open might not be able to handle the
relcache entry changing underneath them.  Therefore, we need to lock
the relation in a way that no one can have the relation open
concurrently.  But for indexes, the relcache handles reloads specially
in RelationReloadIndexInfo() in a way that keeps changes in the
relcache entry to a minimum.  As long as no one keeps pointers to
rd_amcache and rd_options around across possible relcache flushes,
which is the case, this ought to be safe.

We also want to use a self-exclusive lock for correctness, so that
concurrent DDL doesn't overwrite the rename if they start updating
while still seeing the old version.  Therefore, we use
ShareUpdateExclusiveLock, which is already used by other DDL commands
that want to operate in a concurrent manner.

The reason this is interesting at all is that renaming an index is a
typical part of a concurrent reindexing workflow (CREATE INDEX
CONCURRENTLY new + DROP INDEX CONCURRENTLY old + rename back).  And
indeed a future built-in REINDEX CONCURRENTLY might rely on the ability
to do concurrent renames as well.
Reviewed-by: default avatarAndrey Klychkov <aaklychkov@mail.ru>
Reviewed-by: default avatarFabrízio de Royes Mello <fabriziomello@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/1531767486.432607658@f357.i.mail.ru
      1b5d797c
    • Michael Paquier's avatar
      Initialize TransactionState and user ID consistently at transaction start · b4721f39
      Michael Paquier authored 
      If a failure happens when a transaction is starting between the moment
the transaction status is changed from TRANS_DEFAULT to TRANS_START and
the moment the current user ID and security context flags are fetched
via GetUserIdAndSecContext(), or before initializing its basic fields,
then those may get reset to incorrect values when the transaction
aborts, leaving the session in an inconsistent state.

One problem reported is that failing a starting transaction at the first
query of a session could cause several kinds of system crashes on the
follow-up queries.

In order to solve that, move the initialization of the transaction state
fields and the call of GetUserIdAndSecContext() in charge of fetching
the current user ID close to the point where the transaction status is
switched to TRANS_START, where there cannot be any error triggered
in-between, per an idea of Tom Lane.  This properly ensures that the
current user ID, the security context flags and that the basic fields of
TransactionState remain consistent even if the transaction fails while
starting.

Reported-by: Richard Guo
Diagnosed-By: Richard Guo
Author: Michael Paquier
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAN_9JTxECSb=pEPcb0a8d+6J+bDcOZ4=DgRo_B7Y5gRHJUM=Rw@mail.gmail.com
Backpatch-through: 9.4
      b4721f39
    • Michael Paquier's avatar
      Add flag values in WAL description to all heap records · 3be97b97
      Michael Paquier authored 
      Hexadecimal is consistently used as format to not bloat too much the
output but keep it readable.  This information is useful mainly for
debugging purposes with for example pg_waldump.

Author: Michael Paquier
Reviewed-by: Nathan Bossart, Dmitry Dolgov, Andres Freund, Álvaro
Herrera
Discussion: https://postgr.es/m/20180413034734.GE1552@paquier.xyz
      3be97b97
    • Michael Paquier's avatar
      Refactor code creating PartitionBoundInfo · b52b7dc2
      Michael Paquier authored 
      The code building PartitionBoundInfo based on the constituent partition
data read from catalogs has been located in partcache.c, with a specific
set of routines dedicated to bound types, like sorting or bound data
creation.  All this logic is moved to partbounds.c and relocates all the
bound-specific logistic into it, with partition_bounds_create() as
principal entry point.

Author: Amit Langote
Reviewed-by: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/3f289da8-6d10-75fe-814a-635e8b191d43@lab.ntt.co.jp
      b52b7dc2
  5. 13 Nov, 2018 11 commits
  7. 12 Nov, 2018 6 commits
    • Michael Paquier's avatar
      Remove CommandCounterIncrement() after processing ON COMMIT DELETE · 52b70b1c
      Michael Paquier authored 
      This comes from f9b5b41e, which is part of one the original commits that
implemented ON COMMIT actions.  By looking at the truncation code, any
CCI needed happens locally when rebuilding indexes, so it looks safe to
just remove this final incrementation.

Author: Michael Paquier
Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/20181109024731.GF2652@paquier.xyz
      52b70b1c
    • Tom Lane's avatar
      Simplify null-element handling in extension_config_remove(). · 3de49148
      Tom Lane authored 
      There's no point in asking deconstruct_array() for a null-flags
array when we already checked the array has no nulls, and aren't
going to examine the output anyhow.  Not asking for this output
should make the code marginally faster, and it's also more
robust since if there somehow were nulls, deconstruct_array()
would throw an error.

Daniel Gustafsson

Discussion: https://postgr.es/m/289FFB8B-7AAB-48B5-A497-6E0D41D7BA47@yesql.se
      3de49148
    • Tom Lane's avatar
      Limit the number of index clauses considered in choose_bitmap_and(). · e3f005d9
      Tom Lane authored 
      classify_index_clause_usage() is O(N^2) in the number of distinct index
qual clauses it considers, because of its use of a simple search list to
store them.  For nearly all queries, that's fine because only a few clauses
will be considered.  But Alexander Kuzmenkov reported a machine-generated
query with 80000 (!) index qual clauses, which caused this code to take
forever.  Somewhat remarkably, this is the only O(N^2) behavior we now
have for such a query, so let's fix it.

We can get rid of the O(N^2) runtime for cases like this without much
damage to the functionality of choose_bitmap_and() by separating out
paths with "too many" qual or pred clauses, and deeming them to always
be nonredundant with other paths.  Then their clauses needn't go into
the search list, so it doesn't get too long, but we don't lose the
ability to consider bitmap AND plans altogether.  I set the threshold
for "too many" to be 100 clauses per path, which should be plenty to
ensure no change in planning behavior for normal queries.

There are other things we could do to make this go faster, but it's not
clear that it's worth any additional effort.  80000 qual clauses require
a whole lot of work in many other places, too.

The code's been like this for a long time, so back-patch to all supported
branches.  The troublesome query only works back to 9.5 (in 9.4 it fails
with stack overflow in the parser); so I'm not sure that fixing this in
9.4 has any real-world benefit, but perhaps it does.

Discussion: https://postgr.es/m/90c5bdfa-d633-dabe-9889-3cf3e1acd443@postgrespro.ru
      e3f005d9
    • Michael Paquier's avatar
      Fix incorrect author name in release notes · ffb68980
      Michael Paquier authored 
      Author: Alexander Lakhin
Discussion: https://postgr.es/m/2f55f6d2-3fb0-d4f6-5c47-18da3a1117e0@gmail.com
      ffb68980
    • Peter Eisentraut's avatar
      doc: Small punctuation improvement · fc151211
      Peter Eisentraut authored
      fc151211
    • Peter Eisentraut's avatar
      doc: Small run-time pruning doc fix · 253b3f67
      Peter Eisentraut authored 
      A note in ddl.sgml used to mention that run-time pruning was only
implemented for Append.  When we got MergeAppend support, this was
updated to mention that MergeAppend is supported too.  This is
slightly weird as it's not all that obvious what exactly isn't
supported when we mention:

    <para>
     Both of these behaviors are likely to be changed in a future release
     of <productname>PostgreSQL</productname>.
    </para>

This patch updates this to mention that ModifyTable is unsupported,
which makes the above fragment make sense again.

Author: David Rowley <david.rowley@2ndquadrant.com>
      253b3f67
  9. 11 Nov, 2018 2 commits
  11. 10 Nov, 2018 7 commits
    • Peter Eisentraut's avatar
      Apply RI trigger skipping tests also for DELETE · 69ee2ff9
      Peter Eisentraut authored 
      The tests added in cfa0f425 to skip
firing an RI trigger if any old key value is NULL can also be applied
for DELETE.  This should give a performance gain in those cases, and it
also saves a lot of duplicate code in the actual RI triggers.  (That
code was already dead code for the UPDATE cases.)
Reviewed-by: default avatarDaniel Gustafsson <daniel@yesql.se>
      69ee2ff9
    • Peter Eisentraut's avatar
      Remove dead foreign key optimization code · 34479d9a
      Peter Eisentraut authored 
      The ri_KeysEqual() calls in the foreign-key trigger functions to
optimize away some updates are useless because since
adfeef55 those triggers are not enqueued
at all.  (It's also not useful to keep these checks as some kind of
backstop, since it's also semantically correct to just run the full
check even with equal keys.)
Reviewed-by: default avatarDaniel Gustafsson <daniel@yesql.se>
      34479d9a
    • Andres Freund's avatar
      Combine two flag tests in GetSnapshotData(). · 5fde047f
      Andres Freund authored 
      Previously the code checked PROC_IN_LOGICAL_DECODING and
PROC_IN_VACUUM separately. As the relevant variable is marked as
volatile, the compiler cannot combine the two tests.  As
GetSnapshotData() is pretty hot in a number of workloads, it's
worthwhile to fix that.

It'd also be a good idea to get rid of the volatiles altogether. But
for one that's a larger patch, and for another, the code after this
change still seems at least as easy to read as before.

Author: Andres Freund
Discussion: https://postgr.es/m/20181005172955.wyjb4fzcdzqtaxjq@alap3.anarazel.de
      5fde047f
    • Andres Freund's avatar
      docs: Adapt wal_segment_size docs to fc49e24f. · 5fc1670b
      Andres Freund authored 
      Before this change the docs weren't adapted to the fact that
wal_segment_size is now measured in bytes, rather than multiples of
wal_block_size.

Author: David Steele
Discussion: https://postgr.es/m/68ea97d6-2ed9-f339-e57d-ab3a33caf3b1@pgmasters.net
Backpatch: 11-, like fc49e24f itself.
      5fc1670b
    • Tom Lane's avatar
      Fix error-cleanup mistakes in exec_stmt_call(). · f26c06a4
      Tom Lane authored 
      Commit 15c72934 was a couple bricks shy of a load: we need to
ensure that expr->plan gets reset to NULL on any error exit,
if it's not supposed to be saved.  Also ensure that the
stmt->target calculation gets redone if needed.

The easy way to exhibit a problem is to set up code that
violates the writable-argument restriction and then execute
it twice.  But error exits out of, eg, setup_param_list()
could also break it.  Make the existing PG_TRY block cover
all of that code to be sure.

Per report from Pavel Stehule.

Discussion: https://postgr.es/m/CAFj8pRAeXNTO43W2Y0Cn0YOVFPv1WpYyOqQrrzUiN6s=dn7gCg@mail.gmail.com
      f26c06a4
    • Tom Lane's avatar
      Fix missing role dependencies for some schema and type ACLs. · fa2952d8
      Tom Lane authored 
      This patch fixes several related cases in which pg_shdepend entries were
never made, or were lost, for references to roles appearing in the ACLs of
schemas and/or types.  While that did no immediate harm, if a referenced
role were later dropped, the drop would be allowed and would leave a
dangling reference in the object's ACL.  That still wasn't a big problem
for normal database usage, but it would cause obscure failures in
subsequent dump/reload or pg_upgrade attempts, taking the form of
attempts to grant privileges to all-numeric role names.  (I think I've
seen field reports matching that symptom, but can't find any right now.)

Several cases are fixed here:

1. ALTER DOMAIN SET/DROP DEFAULT would lose the dependencies for any
existing ACL entries for the domain.  This case is ancient, dating
back as far as we've had pg_shdepend tracking at all.

2. If a default type privilege applies, CREATE TYPE recorded the
ACL properly but forgot to install dependency entries for it.
This dates to the addition of default privileges for types in 9.2.

3. If a default schema privilege applies, CREATE SCHEMA recorded the
ACL properly but forgot to install dependency entries for it.
This dates to the addition of default privileges for schemas in v10
(commit ab89e465).

Another somewhat-related problem is that when creating a relation
rowtype or implicit array type, TypeCreate would apply any available
default type privileges to that type, which we don't really want
since such an object isn't supposed to have privileges of its own.
(You can't, for example, drop such privileges once they've been added
to an array type.)

ab89e465 is also to blame for a race condition in the regression tests:
privileges.sql transiently installed globally-applicable default
privileges on schemas, which sometimes got absorbed into the ACLs of
schemas created by concurrent test scripts.  This should have resulted
in failures when privileges.sql tried to drop the role holding such
privileges; but thanks to the bug fixed here, it instead led to dangling
ACLs in the final state of the regression database.  We'd managed not to
notice that, but it became obvious in the wake of commit da906766, which
allowed the race condition to occur in pg_upgrade tests.

To fix, add a function recordDependencyOnNewAcl to encapsulate what
callers of get_user_default_acl need to do; while the original call
sites got that right via ad-hoc code, none of the later-added ones
have.  Also change GenerateTypeDependencies to generate these
dependencies, which requires adding the typacl to its parameter list.
(That might be annoying if there are any extensions calling that
function directly; but if there are, they're most likely buggy in the
same way as the core callers were, so they need work anyway.)  While
I was at it, I changed GenerateTypeDependencies to accept most of its
parameters in the form of a Form_pg_type pointer, making its parameter
list a bit less unwieldy and mistake-prone.

The test race condition is fixed just by wrapping the addition and
removal of default privileges into a single transaction, so that that
state is never visible externally.  We might eventually prefer to
separate out tests of default privileges into a script that runs by
itself, but that would be a bigger change and would make the tests
run slower overall.

Back-patch relevant parts to all supported branches.

Discussion: https://postgr.es/m/15719.1541725287@sss.pgh.pa.us
      fa2952d8
    • Andres Freund's avatar
      Remove ineffective check against dropped columns from slot_getattr(). · c670d0fa
      Andres Freund authored 
      Before this commit slot_getattr() checked for dropped
columns (returning NULL in that case), but only after checking for
previously deformed columns. As slot_deform_tuple() does not contain
such a check, the check in slot_getattr() would often not have been
reached, depending on previous use of the slot.

These days locking and plan invalidation ought to ensure that dropped
columns are not accessed in query plans. Therefore this commit just
drops the insufficient check in slot_getattr().  It's possible that
we'll find some holes againt use of dropped columns, but if so, those
need to be addressed independent of slot_getattr(), as most accesses
don't go through that function anyway.

Author: Andres Freund
Discussion: https://postgr.es/m/20181107174403.zai7fedgcjoqx44p@alap3.anarazel.de
      c670d0fa