Skip to content

P
Postgres FD Implementation
Switch branch/tag
  1. 31 Jan, 2020 7 commits
    • Tom Lane's avatar
      Fix CheckAttributeType's handling of collations for ranges. · 74b35eb4
      Tom Lane authored 
      Commit fc769589 changed CheckAttributeType to recurse into ranges,
but made it pass down the wrong collation (always InvalidOid, since
ranges as such have no collation).  This would result in guaranteed
failure when considering a range type whose subtype is collatable.

Embarrassingly, we lack any regression tests that would expose such
a problem (but fortunately, somebody noticed before we shipped this
bug in any release).

Fix it to pass down the range's subtype collation property instead,
and add some regression test cases to exercise collatable-subtype
ranges a bit more.  Back-patch to all supported branches, as the
previous patch was.

Report and patch by Julien Rouhaud, test cases tweaked by me

Discussion: https://postgr.es/m/CAOBaU_aBWqNweiGUFX0guzBKkcfJ8mnnyyGC_KBQmO12Mj5f_A@mail.gmail.com
      74b35eb4
    • Tom Lane's avatar
      Fix parallel pg_dump/pg_restore for failure to create worker processes. · 2425f8f7
      Tom Lane authored 
      If we failed to fork a worker process, or create a communication pipe
for one, WaitForTerminatingWorkers would suffer an assertion failure
if assert-enabled, otherwise crash or go into an infinite loop.  This
was a consequence of not accounting for the startup condition where
we've not yet forked all the workers.

The original bug was that ParallelBackupStart would set workerStatus to
WRKR_IDLE before it had successfully forked a worker.  I made things
worse in commit b7b8cc0c by not understanding the undocumented fact
that the WRKR_TERMINATED state was also meant to represent the case
where a worker hadn't been started yet: I changed enum T_WorkerStatus
so that *all* the worker slots were initially in WRKR_IDLE state.  But
this wasn't any more broken in practice, since even one slot in the
wrong state would keep WaitForTerminatingWorkers from terminating.

In v10 and later, introduce an explicit T_WorkerStatus value for
worker-not-started, in hopes of preventing future oversights of the
same ilk.  Before that, just document that WRKR_TERMINATED is supposed
to cover that case (partly because it wasn't actively broken, and
partly because the enum is exposed outside parallel.c in those branches,
so there's microscopically more risk involved in changing it).
In all branches, introduce a WORKER_IS_RUNNING status test macro
to hide which T_WorkerStatus values mean that, and be more careful
not to access ParallelSlot fields till we're sure they're valid.

Per report from Vignesh C, though this is my patch not his.
Back-patch to all supported branches.

Discussion: https://postgr.es/m/CALDaNm1Luv-E3sarR+-unz-BjchquHHyfP+YC+2FS2pt_J+wxg@mail.gmail.com
      2425f8f7
    • Peter Eisentraut's avatar
      Allow building without default socket directory · a9cff89f
      Peter Eisentraut authored 
      We have code paths for Unix socket support and no Unix socket support.
Now add a third variant: Unix socket support but do not use a Unix
socket by default in the client or the server, only if you explicitly
specify one.  This will be useful when we enable Unix socket support
on Windows.

To implement this, tweak things so that setting DEFAULT_PGSOCKET_DIR
to "" has the desired effect.  This mostly already worked like that;
only a few places needed to be adjusted.  Notably, the reference to
DEFAULT_PGSOCKET_DIR in UNIXSOCK_PATH() could be removed because all
callers already resolve an empty socket directory setting with a
default if appropriate.
Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/75f72249-8ae6-322a-63df-4fe03eeccb9f@2ndquadrant.com
      a9cff89f
    • Peter Eisentraut's avatar
      Sprinkle some const decorations · 7c23bfd2
      Peter Eisentraut authored 
      This might help clarify the API a bit.
      7c23bfd2
    • Michael Paquier's avatar
      Fix typo in recently-added TAP test for replication slots · 7ca8c970
      Michael Paquier authored 
      Oversight in commit b0afdcad.
      7ca8c970
    • Thomas Munro's avatar
      Report time spent in posix_fallocate() as a wait event. · ef02fb15
      Thomas Munro authored 
      When allocating DSM segments with posix_fallocate() on Linux (see commit
899bd785), report this activity as a wait event exactly as we would if
we were using file-backed DSM rather than shm_open()-backed DSM.

Author: Thomas Munro
Discussion: https://postgr.es/m/CA%2BhUKGKCSh4GARZrJrQZwqs5SYp0xDMRr9Bvb%2BHQzJKvRgL6ZA%40mail.gmail.com
      ef02fb15
    • Thomas Munro's avatar
      Adjust DSM and DSA slot usage constants. · d061ea21
      Thomas Munro authored 
      When running a lot of large parallel queries concurrently, or a plan with
a lot of separate Gather nodes, it is possible to run out of DSM slots.
There are better solutions to these problems requiring architectural
redesign work, but for now, let's adjust the constants so that it's more
difficult to hit the limit.

1.  Previously, a DSA area would create up to four segments at each size
before doubling the size.  After this commit, it will create only two at
each size, so it ramps up faster and therefore needs fewer slots.

2.  Previously, the total limit on DSM slots allowed for 2 per connection.
Switch to 5 per connection.

Also remove an obsolete nearby comment.

Author: Thomas Munro
Reviewed-by: Robert Haas, Andres Freund
Discussion: https://postre.es/m/CA%2BhUKGL6H2BpGbiF7Lj6QiTjTGyTLW_vLR%3DSn2tEBeTcYXiMKw%40mail.gmail.com
      d061ea21
  3. 30 Jan, 2020 8 commits
  5. 29 Jan, 2020 7 commits
  7. 28 Jan, 2020 7 commits
  9. 27 Jan, 2020 5 commits
  11. 26 Jan, 2020 4 commits
    • Tom Lane's avatar
      Fix EXPLAIN (SETTINGS) to follow policy about when to print empty fields. · 3ec20c70
      Tom Lane authored 
      In non-TEXT output formats, the "Settings" field should appear when
requested, even if it would be empty.

Also, get rid of the premature optimization of counting all the
GUC_EXPLAIN variables at startup.  Since there was no provision for
adjusting that count later, all it'd take would be some extension marking
a parameter as GUC_EXPLAIN to risk an assertion failure or memory stomp.
We could make get_explain_guc_options() count those variables on-the-fly,
or dynamically resize its array ... but TBH I do not think that making a
transient array of pointers a bit smaller is worth any extra complication,
especially when you consider all the other transient space EXPLAIN eats.
So just allocate that array at the max possible size.

In HEAD, also add some regression test coverage for this feature.

Because of the memory-stomp hazard, back-patch to v12 where this
feature was added.

Discussion: https://postgr.es/m/19416.1580069629@sss.pgh.pa.us
      3ec20c70
    • Thomas Munro's avatar
      Refactor confusing code in _mdfd_openseg(). · f37ff034
      Thomas Munro authored 
      As reported independently by a couple of people, _mdfd_openseg() is coded in a
way that seems to imply that the segments could be opened in an order that
isn't strictly sequential.  Even if that were true, it's also using the wrong
comparison.  It's not an active bug, since the condition is always true anyway,
but it's confusing, so replace it with an assertion.

Author: Thomas Munro
Reviewed-by: Andres Freund, Kyotaro Horiguchi, Noah Misch
Discussion: https://postgr.es/m/CA%2BhUKG%2BNBw%2BuSzxF1os-SO6gUuw%3DcqO5DAybk6KnHKzgGvxhxA%40mail.gmail.com
Discussion: https://postgr.es/m/20191222091930.GA1280238%40rfd.leadboat.com
      f37ff034
    • Tom Lane's avatar
      In postgres_fdw, don't try to ship MULTIEXPR updates to remote server. · 215824f9
      Tom Lane authored 
      In a statement like "UPDATE remote_tab SET (x,y) = (SELECT ...)",
we'd conclude that the statement could be directly executed remotely,
because the sub-SELECT is in a resjunk tlist item that's not examined
for shippability.  Currently that ends up crashing if the sub-SELECT
contains any remote Vars.  Prevent the crash by deeming MULTIEXEC
Params to be unshippable.

This is a bit of a brute-force solution, since if the sub-SELECT
*doesn't* contain any remote Vars, the current execution technology
would work; but that's not a terribly common use-case for this syntax,
I think.  In any case, we generally don't try to ship sub-SELECTs, so
it won't surprise anybody that this doesn't end up as a remote direct
update.  I'd be inclined to see if that general limitation can be fixed
before worrying about this case further.

Per report from Lukáš Sobotka.

Back-patch to 9.6.  9.5 had MULTIEXPR, but we didn't try to perform
remote direct updates then, so the case didn't arise anyway.

Discussion: https://postgr.es/m/CAJif3k+iA_ekBB5Zw2hDBaE1wtiQa4LH4_JUXrrMGwTrH0J01Q@mail.gmail.com
      215824f9
    • Heikki Linnakangas's avatar
      Refactor XLogReadRecord(), adding XLogBeginRead() function. · 38a95731
      Heikki Linnakangas authored 
      The signature of XLogReadRecord() required the caller to pass the starting
WAL position as argument, or InvalidXLogRecPtr to continue reading at the
end of previous record. That's slightly awkward to the callers, as most
of them don't want to randomly jump around in the WAL stream, but start
reading at one position and then read everything from that point onwards.
Remove the 'RecPtr' argument and add a new function XLogBeginRead() to
specify the starting position instead. That's more convenient for the
callers. Also, xlogreader holds state that is reset when you change the
starting position, so having a separate function for doing that feels like
a more natural fit.

This changes XLogFindNextRecord() function so that it doesn't reset the
xlogreader's state to what it was before the call anymore. Instead, it
positions the xlogreader to the found record, like XLogBeginRead().

Reviewed-by: Kyotaro Horiguchi, Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/5382a7a3-debe-be31-c860-cb810c08f366%40iki.fi
      38a95731
  13. 25 Jan, 2020 2 commits
    • Tom Lane's avatar
      Clean up EXPLAIN's handling of per-worker details. · 10013684
      Tom Lane authored 
      Previously, it was possible for EXPLAIN ANALYZE of a parallel query
to produce several different "Workers" fields for a single plan node,
because different portions of explain.c independently generated
per-worker data and wrapped that output in separate fields.  This
is pretty bogus, especially for the structured output formats: even
if it's not technically illegal, most programs would have a hard time
dealing with such data.

To improve matters, add infrastructure that allows redirecting
per-worker values into a side data structure, and then collect that
data into a single "Workers" field after we've finished running all
the relevant code for a given plan node.

There are a few visible side-effects:

* In text format, instead of something like

  Sort Method: external merge  Disk: 4920kB
  Worker 0:  Sort Method: external merge  Disk: 5880kB
  Worker 1:  Sort Method: external merge  Disk: 5920kB
  Buffers: shared hit=682 read=10188, temp read=1415 written=2101
  Worker 0:  actual time=130.058..130.324 rows=1324 loops=1
    Buffers: shared hit=337 read=3489, temp read=505 written=739
  Worker 1:  actual time=130.273..130.512 rows=1297 loops=1
    Buffers: shared hit=345 read=3507, temp read=505 written=744

you get

  Sort Method: external merge  Disk: 4920kB
  Buffers: shared hit=682 read=10188, temp read=1415 written=2101
  Worker 0:  actual time=130.058..130.324 rows=1324 loops=1
    Sort Method: external merge  Disk: 5880kB
    Buffers: shared hit=337 read=3489, temp read=505 written=739
  Worker 1:  actual time=130.273..130.512 rows=1297 loops=1
    Sort Method: external merge  Disk: 5920kB
    Buffers: shared hit=345 read=3507, temp read=505 written=744

* When JIT is enabled, any relevant per-worker JIT stats are attached
to the child node of the Gather or Gather Merge node, which is where
the other per-worker output has always been.  Previously, that info
was attached directly to a Gather node, or missed entirely for Gather
Merge.

* A query's summary JIT data no longer includes a bogus
"Worker Number: -1" field.

A notable code-level change is that indenting for lines of text-format
output should now be handled by calling "ExplainIndentText(es)",
instead of hard-wiring how much space to emit.  This seems a good deal
cleaner anyway.

This patch also adds a new "explain.sql" regression test script that's
dedicated to testing EXPLAIN.  There is more that can be done in that
line, certainly, but for now it just adds some coverage of the XML and
YAML output formats, which had been completely untested.

Although this is surely a bug fix, it's not clear that people would
be happy with rearranging EXPLAIN output in a minor release, so apply
to HEAD only.

Maciek Sakrejda and Tom Lane, based on an idea of Andres Freund's;
reviewed by Georgios Kokolatos

Discussion: https://postgr.es/m/CAOtHd0AvAA8CLB9Xz0wnxu1U=zJCKrr1r4QwwXi_kcQsHDVU=Q@mail.gmail.com
      10013684
    • Dean Rasheed's avatar
      Add functions gcd() and lcm() for integer and numeric types. · 13661ddd
      Dean Rasheed authored 
      These compute the greatest common divisor and least common multiple of
a pair of numbers using the Euclidean algorithm.

Vik Fearing, reviewed by Fabien Coelho.

Discussion: https://postgr.es/m/adbd3e0b-e3f1-5bbc-21db-03caf1cef0f7@2ndquadrant.com
      13661ddd