Re: Minimal logical decoding on standbys
Hi, On 2023-03-31 12:45:51 +0200, Drouvot, Bertrand wrote: > On 3/31/23 6:33 AM, Andres Freund wrote: > > Hi, > > > > On 2023-03-30 18:23:41 +0200, Drouvot, Bertrand wrote: > > > On 3/30/23 9:04 AM, Andres Freund wrote: > > > > I think this commit is ready to go. Unless somebody thinks differently, > > > > I > > > > think I might push it tomorrow. > > > > > > Great! Once done, I'll submit a new patch so that GlobalVisTestFor() can > > > make > > > use of the heap relation in vacuumRedirectAndPlaceholder() (which will be > > > possible > > > once 0001 is committed). > > > > Unfortunately I did find an issue doing a pre-commit review of the patch. > > > > The patch adds VISIBILITYMAP_IS_CATALOG_REL to xl_heap_visible.flags - but > > it > > does not remove the bit before calling visibilitymap_set(). > > > > This ends up corrupting the visibilitymap, because the we'll set a bit for > > another page. > > > > Oh I see, I did not think about that (not enough experience in the VM area). > Nice catch and thanks for pointing out! I pushed a commit just adding an assertion that only valid bits are passed in. > > I'm also thinking of splitting the patch into two. One patch to pass down > > the > > heap relation into the new places, and another for the rest. > > I think that makes sense. I don't know how far you've work on the split but > please > find attached V54 doing such a split + implementing your > VISIBILITYMAP_XLOG_VALID_BITS > suggestion. I pushed the pass-the-relation part. I removed an include of catalog.h that was in the patch - I suspect it might have slipped in there from a later patch in the series... I was a bit bothered by using 'heap' instead of 'table' in so many places (eventually we imo should standardize on the latter), but looking around the changed places, heap was used for things like buffers etc. So I left it at heap. Glad we split 0001 - the rest is a lot easier to review. Greetings, Andres Freund
Re: POC: Lock updated tuples in tuple_update() and tuple_delete()
Hi, On 2023-04-02 03:37:19 +0300, Alexander Korotkov wrote: > On Sat, Apr 1, 2023 at 8:21 AM Andres Freund wrote: > > Given that the in-tree state has been broken for a week, I think it probably > > is time to revert the commits that already went in. > > The revised patch is attached. The most notable change is getting rid > of LazyTupleTableSlot. Also get rid of complex computations to detect > how to initialize LazyTupleTableSlot. Instead just pass the oldSlot > as an argument of ExecUpdate() and ExecDelete(). The price for this > is just preallocation of ri_oldTupleSlot before calling ExecDelete(). > The slot allocation is quite cheap. After all wrappers it's > table_slot_callbacks(), which is very cheap, single palloc() and few > fields initialization. It doesn't seem reasonable to introduce an > infrastructure to evade this. > > I think patch resolves all the major issues you've highlighted. Even > if there are some minor things missed, I'd prefer to push this rather > than reverting the whole work. Shrug. You're designing new APIs, days before the feature freeze. This just doesn't seem ready in time for 16. I certainly won't have time to look at it sufficiently in the next 5 days. Greetings, Andres Freund
Re: POC: Lock updated tuples in tuple_update() and tuple_delete()
.Hi! On Sat, Apr 1, 2023 at 8:21 AM Andres Freund wrote: > On 2023-03-31 16:57:41 +0300, Alexander Korotkov wrote: > > On Wed, Mar 29, 2023 at 8:34 PM Alexander Korotkov > > wrote: > > > On Mon, Mar 27, 2023 at 1:49 PM Alexander Korotkov > > > wrote: > > > > On Fri, Mar 24, 2023 at 3:39 AM Andres Freund > > > > wrote: > > > > > On 2023-03-23 23:24:19 +0300, Alexander Korotkov wrote: > > > > > > On Thu, Mar 23, 2023 at 8:06 PM Andres Freund > > > > > > wrote: > > > > > > > I seriously doubt that solving this at the tuple locking level is > > > > > > > the right > > > > > > > thing. If we want to avoid refetching tuples, why don't we add a > > > > > > > parameter to > > > > > > > delete/update to generally put the old tuple version into a slot, > > > > > > > not just as > > > > > > > an optimization for a subsequent lock_tuple()? Then we could > > > > > > > remove all > > > > > > > refetching tuples for triggers. It'd also provide the basis for > > > > > > > adding support > > > > > > > for referencing the OLD version in RETURNING, which'd be quite > > > > > > > powerful. > > > > > > > > After some thoughts, I think I like idea of fetching old tuple version > > > > in update/delete. Everything that evades extra tuple fetching and do > > > > more of related work in a single table AM call, makes table AM API > > > > more flexible. > > > > > > > > I'm working on patch implementing this. I'm going to post it later > > > > today. > > > > > > Here is the patchset. I'm continue to work on comments and refactoring. > > > > > > My quick question is why do we need ri_TrigOldSlot for triggers? > > > Can't we just pass the old tuple for after row trigger in > > > ri_oldTupleSlot? > > > > > > Also, I wonder if we really need a LazyTupleSlot. It allows to evade > > > extra tuple slot allocation. But as I get in the end the tuple slot > > > allocation is just a single palloc. I bet the effect would be > > > invisible in the benchmarks. > > > > Sorry, previous patches don't even compile. The fixed version is attached. > > I'm going to post significantly revised patchset soon. > > Given that the in-tree state has been broken for a week, I think it probably > is time to revert the commits that already went in. The revised patch is attached. The most notable change is getting rid of LazyTupleTableSlot. Also get rid of complex computations to detect how to initialize LazyTupleTableSlot. Instead just pass the oldSlot as an argument of ExecUpdate() and ExecDelete(). The price for this is just preallocation of ri_oldTupleSlot before calling ExecDelete(). The slot allocation is quite cheap. After all wrappers it's table_slot_callbacks(), which is very cheap, single palloc() and few fields initialization. It doesn't seem reasonable to introduce an infrastructure to evade this. I think patch resolves all the major issues you've highlighted. Even if there are some minor things missed, I'd prefer to push this rather than reverting the whole work. -- Regards, Alexander Korotkov 0001-Revise-changes-in-764da7710b-and-11470f544e-v3.patch Description: Binary data
Re: Non-superuser subscription owners
Hi,
On April 1, 2023 9:00:00 AM PDT, Alexander Lakhin wrote:
>Hello Robert,
>
>31.03.2023 23:00, Robert Haas wrote:
>> That looks like a reasonable fix but I can't reproduce the problem
>> locally. I thought the reason why that machine sees the problem might
>> be that it uses -DRELCACHE_FORCE_RELEASE, but I tried that option here
>> and the tests still pass. Anyone ideas how to reproduce?
>
>I've managed to reproduce it using the following script:
>for ((i=1;i<=10;i++)); do
>echo "iteration $i"
>echo "
>CREATE ROLE sub_user;
>CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db'
> PUBLICATION testpub WITH (connect = false);
>ALTER SUBSCRIPTION testsub ENABLE;
>DROP SUBSCRIPTION testsub;
>SELECT pg_sleep(0.001);
>DROP ROLE sub_user;
>" | psql
>psql -c "ALTER SUBSCRIPTION testsub DISABLE;"
>psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);"
>psql -c "DROP SUBSCRIPTION testsub;"
>grep 'TRAP' server.log && break
>done
>
>iteration 3
>CREATE ROLE
>...
>ALTER SUBSCRIPTION
>WARNING: terminating connection because of crash of another server process
>DETAIL: The postmaster has commanded this server process to roll back the
>current transaction and exit, because ano
>ther server process exited abnormally and possibly corrupted shared memory.
>HINT: In a moment you should be able to reconnect to the database and repeat
>your command.
>server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
>connection to server was lost
>TRAP: failed Assert("IsTransactionState()"), File: "catcache.c", Line: 1208,
>PID: 1001242
Errors like that are often easier to reproduce with clobber caches (or whatever
the name is these days) enabled.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: zstd compression for pg_dump
On 4/1/23 15:36, Justin Pryzby wrote: > > ... > >> If there are no concerns, why disable it outside Windows? I don't have a >> good idea how beneficial the multi-threaded compression is, so I can't >> quite judge the risk/benefits tradeoff. > > Because it's a minor/fringe feature, and it's annoying to have platform > differences (would we plan on relaxing the restriction in v17, or is it > more likely we'd forget ?). > > I realized how little I've tested with zstd workers myself. And I think > on cirrusci, the macos and freebsd tasks have zstd libraries with > threading support, but it wasn't being exercised (because using :workers > would cause the patch to fail unless it's supported everywhere). So I > updated the "for CI only" patch to 1) use meson wraps to compile zstd > library with threading on linux and windows; and, 2) use zstd:workers=3 > "opportunistically" (but avoid failing if threads are not supported, > since the autoconf task still doesn't have access to a library with > thread support). That's a great step, but it still seems bad that the > thread stuff has been little exercised until now. (Also, the windows > task failed; I think that's due to a transient network issue). > Agreed, let's leave the threading for PG17, depending on how beneficial it turns out to be for pg_dump. > Feel free to mess around with threads (but I'd much rather see the patch > progress for zstd:long). OK, understood. The long mode patch is pretty simple. IIUC it does not change the format, i.e. in the worst case we could leave it for PG17 too. Correct? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
On Sat, Apr 01, 2023 at 01:29:13PM -0400, Melanie Plageman wrote: > Hi, > > I was just doing some cleanup on the main patch in this set and realized > that it was missing a few things. One of which is forbidding the > BUFFER_USAGE_LIMIT with VACUUM FULL since VACUUM FULL does not use a > BAS_VACUUM strategy. > > VACUUM FULL technically uses a bulkread buffer access strategy for > reading the original relation if its number of blocks is > number of > shared buffers / 4 (see initscan()). The new rel writing is done using > smgrextend/write directly and doesn't go through shared buffers. I > think it is a stretch to try and use the size passed in to VACUUM by > BUFFER_USAGE_LIMIT for the bulkread strategy ring. When you say that it's a stretch, do you mean that it'd be a pain to add arguments to handful of functions to pass down the setting ? Or that it's unclear if doing so would be the desirable/needed/intended/expected behavior ? I think if VACUUM FULL were going to allow a configurable strategy size, then so should CLUSTER. But it seems fine if they don't. I wonder if maybe strategy should be configurable in some more generic way, like a GUC. At one point I had a patch to allow INSERT to use strategy buffers (not just INSERT SELECT). And that's still pretty desirable. Also COPY. I've seen load spikes caused by pg_dumping tables which are just below 25% of shared_buffers. Which is exacerbated because pg_dump deliberately orders tables by size, so those tables are dumped one after another, each causing eviction of ~20% of shared buffers. And exacerbated some more because TOAST don't seem to use a ring buffer in that case. > I somehow feel like VACUUM (FULL, BUFFER_USAGE_LIMIT 'x') should error > out instead of silently not using the buffer usage limit, though. > > I am looking for others' opinions. Sorry, no opinion here :) One thing is that it's fine to take something that previously throw an error and change it to not throw an error anymore. But it's undesirable to do the opposite. For that reason, there's may be a tendency to add errors for cases like this. -- Justin
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
Hi, I was just doing some cleanup on the main patch in this set and realized that it was missing a few things. One of which is forbidding the BUFFER_USAGE_LIMIT with VACUUM FULL since VACUUM FULL does not use a BAS_VACUUM strategy. VACUUM FULL technically uses a bulkread buffer access strategy for reading the original relation if its number of blocks is > number of shared buffers / 4 (see initscan()). The new rel writing is done using smgrextend/write directly and doesn't go through shared buffers. I think it is a stretch to try and use the size passed in to VACUUM by BUFFER_USAGE_LIMIT for the bulkread strategy ring. As for forbidding the combination, I noticed that when VACUUM FULL is specified with INDEX_CLEANUP OFF, there is no syntax error but the INDEX_CLEANUP option is simply ignored. This is documented behavior. I somehow feel like VACUUM (FULL, BUFFER_USAGE_LIMIT 'x') should error out instead of silently not using the buffer usage limit, though. I am looking for others' opinions. - Melanie
Re: Non-superuser subscription owners
Hello Robert,
31.03.2023 23:00, Robert Haas wrote:
That looks like a reasonable fix but I can't reproduce the problem
locally. I thought the reason why that machine sees the problem might
be that it uses -DRELCACHE_FORCE_RELEASE, but I tried that option here
and the tests still pass. Anyone ideas how to reproduce?
I've managed to reproduce it using the following script:
for ((i=1;i<=10;i++)); do
echo "iteration $i"
echo "
CREATE ROLE sub_user;
CREATE SUBSCRIPTION testsub CONNECTION 'dbname=db'
PUBLICATION testpub WITH (connect = false);
ALTER SUBSCRIPTION testsub ENABLE;
DROP SUBSCRIPTION testsub;
SELECT pg_sleep(0.001);
DROP ROLE sub_user;
" | psql
psql -c "ALTER SUBSCRIPTION testsub DISABLE;"
psql -c "ALTER SUBSCRIPTION testsub SET (slot_name = NONE);"
psql -c "DROP SUBSCRIPTION testsub;"
grep 'TRAP' server.log && break
done
iteration 3
CREATE ROLE
...
ALTER SUBSCRIPTION
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the
current transaction and exit, because ano
ther server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat
your command.
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost
TRAP: failed Assert("IsTransactionState()"), File: "catcache.c", Line: 1208,
PID: 1001242
Best regards,
Alexander
Re: BUG #17877: Referencing a system column in a foreign key leads to incorrect memory access
Em sex., 31 de mar. de 2023 às 16:25, Tom Lane escreveu:
> Ranier Vilela writes:
> > I think that commit f0d65c0
> > <
> https://github.com/postgres/postgres/commit/f0d65c0eaf05d6acd3ae05cde4a31465eb3992b2
> >
> > has an oversight.
> > Attnum == 0, is system column too, right?
>
> No, it's not valid in pg_attribute rows.
>
> > All other places at tablecmds.c, has this test:
> > if (attnum <= 0)
> > ereport(ERROR,
>
> I was actually copying this code in indexcmds.c:
>
> if (attno < 0)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("index creation on system columns is not
> supported")));
>
> There's really no reason to prefer one over the other in this context.
>
I think the documentation is a bit confusing.
According to the current documentation:
/*
* attnum is the "attribute number" for the attribute: A value that
* uniquely identifies this attribute within its class. for user
* attributes, Attribute numbers are greater than 0 and not greater than
* the number of attributes in the class. i.e. if the Class pg_class says
* that Class XYZ has 10 attributes, then the user attribute numbers in
* Class pg_attribute must be 1-10.
*
* System attributes have attribute numbers less than 0 that are unique
* within the class, but not constrained to any particular range.
*
* Note that (attnum - 1) is often used as the index to an array.
Attributes equal to zero are in limbo.
IMO should be:
* System attributes have attribute numbers less or equal to 0 that are
* unique
* within the class, but not constrained to any particular range.
regards,
Ranier Vilela
Re: zstd compression for pg_dump
On Sat, Apr 01, 2023 at 02:49:44PM +0200, Tomas Vondra wrote: > On 4/1/23 02:28, Justin Pryzby wrote: > > On Sat, Apr 01, 2023 at 02:11:12AM +0200, Tomas Vondra wrote: > >> On 4/1/23 01:16, Justin Pryzby wrote: > >>> On Tue, Mar 28, 2023 at 06:23:26PM +0200, Tomas Vondra wrote: > On 3/27/23 19:28, Justin Pryzby wrote: > > On Fri, Mar 17, 2023 at 03:43:31AM +0100, Tomas Vondra wrote: > >> On 3/16/23 05:50, Justin Pryzby wrote: > >>> On Fri, Mar 10, 2023 at 12:48:13PM -0800, Jacob Champion wrote: > On Wed, Mar 8, 2023 at 10:59 AM Jacob Champion > wrote: > > I did some smoke testing against zstd's GitHub release on Windows. > > To > > build against it, I had to construct an import library, and put that > > and the DLL into the `lib` folder expected by the MSVC scripts... > > which makes me wonder if I've chosen a harder way than necessary? > > It looks like pg_dump's meson.build is missing dependencies on zstd > (meson couldn't find the headers in the subproject without them). > >>> > >>> I saw that this was added for LZ4, but I hadn't added it for zstd > >>> since > >>> I didn't run into an issue without it. Could you check that what I've > >>> added works for your case ? > >>> > > Parallel zstd dumps seem to work as expected, in that the resulting > > pg_restore output is identical to uncompressed dumps and nothing > > explodes. I haven't inspected the threading implementation for > > safety > > yet, as you mentioned. > > Hm. Best I can tell, the CloneArchive() machinery is supposed to be > handling safety for this, by isolating each thread's state. I don't > feel > comfortable pronouncing this new addition safe or not, because I'm > not > sure I understand what the comments in the format-specific _Clone() > callbacks are saying yet. > >>> > >>> My line of reasoning for unix is that pg_dump forks before any calls > >>> to > >>> zstd. Nothing zstd does ought to affect the pg_dump layer. But that > >>> doesn't apply to pg_dump under windows. This is an opened question. > >>> If > >>> there's no solid answer, I could disable/ignore the option (maybe only > >>> under windows). > >> > >> I may be missing something, but why would the patch affect this? Why > >> would it even affect safety of the parallel dump? And I don't see any > >> changes to the clone stuff ... > > > > zstd supports using threads during compression, with -Z zstd:workers=N. > > When unix forks, the child processes can't do anything to mess up the > > state of the parent processes. > > > > But windows pg_dump uses threads instead of forking, so it seems > > possible that the pg_dump -j threads that then spawn zstd threads could > > "leak threads" and break the main thread. I suspect there's no issue, > > but we still ought to verify that before declaring it safe. > > OK. I don't have access to a Windows machine so I can't test that. Is it > possible to disable the zstd threading, until we figure this out? > >>> > >>> I think that's what's best. I made it issue a warning if "workers" was > >>> specified. It could also be an error, or just ignored. > >>> > >>> I considered disabling workers only for windows, but realized that I > >>> haven't tested with threads myself - my local zstd package is compiled > >>> without threading, and I remember having some issue recompiling it with > >>> threading. Jacob's recipe for using meson wraps works well, but it > >>> still seems better to leave it as a future feature. I used that recipe > >>> to enabled zstd with threading on CI (except for linux/autoconf). > >> > >> +1 to disable this if we're unsure it works correctly. I agree it's > >> better to just error out if workers are requested - I rather dislike > >> when a tool just ignores an explicit parameter. And AFAICS it's what > >> zstd does too, when someone requests workers on incompatible build. > >> > >> FWIW I've been thinking about this a bit more and I don't quite see why > >> would the threading cause issues (except for Windows). I forgot > >> pg_basebackup already supports zstd, including the worker threading, so > >> why would it work there and not in pg_dump? Sure, pg_basebackup is not > >> parallel, but with separate pg_dump processes that shouldn't be an issue > >> (although I'm not sure when zstd creates threads). > > > > There's no concern at all except under windows (because on windows > > pg_dump -j is implemented using threads rather than forking). > > Especially since zstd:workers is already allowed in the basebackup > > backend process. > > If there are no concerns, why disable it outside Windows? I don't have a > good idea how beneficial the multi-threaded compression is, so I
Re: is_superuser is not documented
On Wed, Mar 29, 2023 at 5:21 PM Bruce Momjian wrote:
>
>On Thu, Mar 2, 2023 at 12:00:43PM -0500, Joseph Koshakow wrote:
>>
>>
>> On Thu, Mar 2, 2023 at 11:53 AM Fujii Masao <
masao.fu...@oss.nttdata.com>
>> wrote:
>> >
>> >On 2022/09/14 14:27, bt22kawamotok wrote:
>> >> I update patch to reflect master update.
>> >
>> >Thanks for updating the patch!
>> >
>> >+
>> >+Shows whether the current user is a superuser or not.
>> >+
>> >
>> >How about adding the note about when this parameter can change,
>> >like we do for in_hot_standby docs? I applied this change to
the patch.
>> >Attached is the updated version of the patch.
>> >
>>
>> I just came across this thread and noticed that the patch was never
>> merged. There is some brief docs for is_superuser in the SHOW docs:
>> https://www.postgresql.org/docs/current/sql-show.html, but the GUC
>> fields were never updated.
>>
>> Is there a reason that it never got merged or was it just forgotten
>> about?
>
>Uh, where are you looking? I see it in the SGML, and in the PG 15
docs:
>
>https://www.postgresql.org/docs/current/sql-show.html
>
>IS_SUPERUSER
>
>True if the current role has superuser privileges.
The patch updated the guc table for is_superuser in
src/backend/utils/misc/guc_tables.c
- /* Not for general use --- used by SET SESSION AUTHORIZATION */
- {"is_superuser", PGC_INTERNAL, UNGROUPED,
+ {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the current user is a superuser."),
NULL,
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
However, when I look at the code on master I don't see this update
/* Not for general use --- used by SET SESSION AUTHORIZATION */
{"is_superuser", PGC_INTERNAL, UNGROUPED,
gettext_noop("Shows whether the current user is a superuser."),
NULL,
GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
Similarly, when running `SHOW ALL` against master I don't see the
is_superuser variable
$ /usr/local/pgsql/bin/psql -c "SHOW ALL" test | grep is_superuser
$
Re: zstd compression for pg_dump
On 4/1/23 02:28, Justin Pryzby wrote: > On Sat, Apr 01, 2023 at 02:11:12AM +0200, Tomas Vondra wrote: >> On 4/1/23 01:16, Justin Pryzby wrote: >>> On Tue, Mar 28, 2023 at 06:23:26PM +0200, Tomas Vondra wrote: On 3/27/23 19:28, Justin Pryzby wrote: > On Fri, Mar 17, 2023 at 03:43:31AM +0100, Tomas Vondra wrote: >> On 3/16/23 05:50, Justin Pryzby wrote: >>> On Fri, Mar 10, 2023 at 12:48:13PM -0800, Jacob Champion wrote: On Wed, Mar 8, 2023 at 10:59 AM Jacob Champion wrote: > I did some smoke testing against zstd's GitHub release on Windows. To > build against it, I had to construct an import library, and put that > and the DLL into the `lib` folder expected by the MSVC scripts... > which makes me wonder if I've chosen a harder way than necessary? It looks like pg_dump's meson.build is missing dependencies on zstd (meson couldn't find the headers in the subproject without them). >>> >>> I saw that this was added for LZ4, but I hadn't added it for zstd since >>> I didn't run into an issue without it. Could you check that what I've >>> added works for your case ? >>> > Parallel zstd dumps seem to work as expected, in that the resulting > pg_restore output is identical to uncompressed dumps and nothing > explodes. I haven't inspected the threading implementation for safety > yet, as you mentioned. Hm. Best I can tell, the CloneArchive() machinery is supposed to be handling safety for this, by isolating each thread's state. I don't feel comfortable pronouncing this new addition safe or not, because I'm not sure I understand what the comments in the format-specific _Clone() callbacks are saying yet. >>> >>> My line of reasoning for unix is that pg_dump forks before any calls to >>> zstd. Nothing zstd does ought to affect the pg_dump layer. But that >>> doesn't apply to pg_dump under windows. This is an opened question. If >>> there's no solid answer, I could disable/ignore the option (maybe only >>> under windows). >> >> I may be missing something, but why would the patch affect this? Why >> would it even affect safety of the parallel dump? And I don't see any >> changes to the clone stuff ... > > zstd supports using threads during compression, with -Z zstd:workers=N. > When unix forks, the child processes can't do anything to mess up the > state of the parent processes. > > But windows pg_dump uses threads instead of forking, so it seems > possible that the pg_dump -j threads that then spawn zstd threads could > "leak threads" and break the main thread. I suspect there's no issue, > but we still ought to verify that before declaring it safe. OK. I don't have access to a Windows machine so I can't test that. Is it possible to disable the zstd threading, until we figure this out? >>> >>> I think that's what's best. I made it issue a warning if "workers" was >>> specified. It could also be an error, or just ignored. >>> >>> I considered disabling workers only for windows, but realized that I >>> haven't tested with threads myself - my local zstd package is compiled >>> without threading, and I remember having some issue recompiling it with >>> threading. Jacob's recipe for using meson wraps works well, but it >>> still seems better to leave it as a future feature. I used that recipe >>> to enabled zstd with threading on CI (except for linux/autoconf). >> >> +1 to disable this if we're unsure it works correctly. I agree it's >> better to just error out if workers are requested - I rather dislike >> when a tool just ignores an explicit parameter. And AFAICS it's what >> zstd does too, when someone requests workers on incompatible build. >> >> FWIW I've been thinking about this a bit more and I don't quite see why >> would the threading cause issues (except for Windows). I forgot >> pg_basebackup already supports zstd, including the worker threading, so >> why would it work there and not in pg_dump? Sure, pg_basebackup is not >> parallel, but with separate pg_dump processes that shouldn't be an issue >> (although I'm not sure when zstd creates threads). > > There's no concern at all except under windows (because on windows > pg_dump -j is implemented using threads rather than forking). > Especially since zstd:workers is already allowed in the basebackup > backend process. > If there are no concerns, why disable it outside Windows? I don't have a good idea how beneficial the multi-threaded compression is, so I can't quite judge the risk/benefits tradeoff. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: POC: Lock updated tuples in tuple_update() and tuple_delete()
Hi, Andres! On Sat, 1 Apr 2023, 09:21 Andres Freund, wrote: > Given that the in-tree state has been broken for a week, I think it > probably > is time to revert the commits that already went in. > It seems that although the patch addressing the issues is not a quick fix, there is a big progress in it already. I propose to see it's status a week later and if it is not ready then to revert existing. Hope there are no other patches in the existing branch complained to suffer this. Kind regards, Pavel Borisov, Supabase >
