public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v56 6/6] Exclude pg_stat directory from base backup 11+ messages / 8 participants [nested] [flat]
* [PATCH v56 6/6] Exclude pg_stat directory from base backup @ 2020-09-29 14:19 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Kyotaro Horiguchi @ 2020-09-29 14:19 UTC (permalink / raw) basebackup sends the content of pg_stat directory, which is doomed to be removed at startup from the backup. Now that pg_stat_statements saves a temporary file into the directory, let exclude pg_stat directory from a base backup. --- src/backend/replication/basebackup.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index f0d75f55bd..f54fcec70a 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -149,6 +149,13 @@ struct exclude_list_item */ static const char *const excludeDirContents[] = { + /* + * Skip statistics files. PGSTAT_STAT_PERMANENT_DIRECTORY must be skipped + * because the files in the directory will be removed at startup from the + * backup. + */ + PGSTAT_STAT_PERMANENT_DIRECTORY, + /* * It is generally not useful to backup the contents of this directory * even if the intention is to restore to another primary. See backup.sgml -- 2.27.0 ----Next_Part(Tue_Mar_16_10_27_55_2021_500)---- ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-14 22:41 Noah Misch <[email protected]> 0 siblings, 5 replies; 11+ messages in thread From: Noah Misch @ 2024-11-14 22:41 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers On Thu, Nov 14, 2024 at 09:33:32PM +0100, Alvaro Herrera wrote: > On 2024-Nov-14, Tom Lane wrote: > > Alvaro Herrera <[email protected]> writes: > > > But timescale did crash: > > > > So what is timescale doing differently? > src/ts_catalog/catalog.c-extern TSDLLEXPORT ResultRelInfo * > src/ts_catalog/catalog.c-ts_catalog_open_indexes(Relation heapRel) > src/ts_catalog/catalog.c-{ > src/ts_catalog/catalog.c- ResultRelInfo *resultRelInfo; > src/ts_catalog/catalog.c- > src/ts_catalog/catalog.c: resultRelInfo = makeNode(ResultRelInfo); > src/ts_catalog/catalog.c- resultRelInfo->ri_RangeTableIndex = 0; /* dummy */ > src/ts_catalog/catalog.c- resultRelInfo->ri_RelationDesc = heapRel; > src/ts_catalog/catalog.c- resultRelInfo->ri_TrigDesc = NULL; /* we don't fire triggers */ > src/ts_catalog/catalog.c- > src/ts_catalog/catalog.c- ExecOpenIndices(resultRelInfo, false); This is a copy of PostgreSQL's CatalogOpenIndexes(). Assuming timescaledb uses it like PostgreSQL uses CatalogOpenIndexes(), it's fine. Specifically, CatalogOpenIndexes() is fine since PostgreSQL does not pass the ResultRelInfo to ModifyTable. > Oh, hmm, there's this bit which uses struct assignment into a stack > element: Yes, stack allocation of a ResultRelInfo sounds relevant to the crash. It qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a sense. I'm hearing the only confirmed impact on non-assert builds is the need to recompile timescaledb. (It's unknown whether recompiling will suffice for timescaledb. For assert builds, six PGXN extensions need recompilation.) I don't see us issuing another set of back branch releases for the purpose of making a v16.4-built timescaledb avoid a rebuild. The target audience would be someone who can get a new PostgreSQL build but can't get a new timescaledb build. That feels like a small audience. What's missing in that analysis? Going forward, for struct field additions, I plan to make my own patches more ABI-breakage-averse than the postgr.es/c/e54a42a standard. ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 00:01 Gregory Smith <[email protected]> parent: Noah Misch <[email protected]> 4 siblings, 0 replies; 11+ messages in thread From: Gregory Smith @ 2024-11-15 00:01 UTC (permalink / raw) To: [email protected]; +Cc: Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers On Thu, Nov 14, 2024 at 5:41 PM Noah Misch <[email protected]> wrote: > I'm hearing the only confirmed impact on non-assert builds is the need to > recompile timescaledb. (It's unknown whether recompiling will suffice for > timescaledb. For assert builds, six PGXN extensions need recompilation.) > That matches what our build and test teams are seeing. We dug into the two lines of impacted Citus code, they are just touching columnar metadata. We dragged Marco into a late night session to double check that with the Citus columnar regression tests and look for red flags in the code. In an assert Citus built against 16.4 running against PostgreSQL 16.5, he hit the assert warnings, but the tests pass and there's no signs or suspicion of a functional impact: CREATE TABLE columnar_table_1 (a int) USING columnar; INSERT INTO columnar_table_1 VALUES (1); +WARNING: problem in alloc set Stripe Write Memory Context: detected write past chunk end in block 0x563ee43a4f10, chunk 0x563ee43a6240 +WARNING: problem in alloc set Stripe Write Memory Context: detected write past chunk end in block 0x563ee4369bb0, chunk 0x563ee436acb0 +WARNING: problem in alloc set Stripe Write Memory Context: detected write past chunk end in block 0x563ee4369bb0, chunk 0x563ee436b3c8 Thanks to everyone who's jumped in to investigate here. With the PL/Perl CVE at an 8.8, sorting out how to get that fix to everyone and navigate the breakage is very important. -- Greg Smith, Crunchy Data Director of Open Source Strategy ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 08:56 Aleksander Alekseev <[email protected]> parent: Noah Misch <[email protected]> 4 siblings, 1 reply; 11+ messages in thread From: Aleksander Alekseev @ 2024-11-15 08:56 UTC (permalink / raw) To: pgsql-hackers; +Cc: Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]> Hi, > > src/ts_catalog/catalog.c-extern TSDLLEXPORT ResultRelInfo * > > src/ts_catalog/catalog.c-ts_catalog_open_indexes(Relation heapRel) > > src/ts_catalog/catalog.c-{ > > src/ts_catalog/catalog.c- ResultRelInfo *resultRelInfo; > > src/ts_catalog/catalog.c- > > src/ts_catalog/catalog.c: resultRelInfo = makeNode(ResultRelInfo); > > src/ts_catalog/catalog.c- resultRelInfo->ri_RangeTableIndex = 0; /* dummy */ > > src/ts_catalog/catalog.c- resultRelInfo->ri_RelationDesc = heapRel; > > src/ts_catalog/catalog.c- resultRelInfo->ri_TrigDesc = NULL; /* we don't fire triggers */ > > src/ts_catalog/catalog.c- > > src/ts_catalog/catalog.c- ExecOpenIndices(resultRelInfo, false); > > This is a copy of PostgreSQL's CatalogOpenIndexes(). Assuming timescaledb > uses it like PostgreSQL uses CatalogOpenIndexes(), it's fine. Specifically, > CatalogOpenIndexes() is fine since PostgreSQL does not pass the ResultRelInfo > to ModifyTable. Yes. Initially I thought this was done for compatibility reasons with different PG versions, but this doesn't seem to be the case since CatalogOpenIndexes() was in the core for a long time. > > Oh, hmm, there's this bit which uses struct assignment into a stack > > element: > > Yes, stack allocation of a ResultRelInfo sounds relevant to the crash. It > qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a sense. Yes, this is another issue. I'm working with the TimescaleDB dev team to fix these issues on the TimescaleDB side. -- Best regards, Aleksander Alekseev ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 11:31 Alvaro Herrera <[email protected]> parent: Noah Misch <[email protected]> 4 siblings, 0 replies; 11+ messages in thread From: Alvaro Herrera @ 2024-11-15 11:31 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers On 2024-Nov-14, Noah Misch wrote: > I'm hearing the only confirmed impact on non-assert builds is the need to > recompile timescaledb. (It's unknown whether recompiling will suffice for > timescaledb. For assert builds, six PGXN extensions need recompilation.) I > don't see us issuing another set of back branch releases for the purpose of > making a v16.4-built timescaledb avoid a rebuild. The target audience would > be someone who can get a new PostgreSQL build but can't get a new timescaledb > build. That feels like a small audience. What's missing in that analysis? I agree with your conclusion that no rewrap is needed. I previously said otherwise, based on claims that there were multiple extensions causing crashes. If the one crash we know about is because timescaledb is using an unusual coding pattern, they can fix that more easily than we can. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo) ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 11:50 Marco Slot <[email protected]> parent: Aleksander Alekseev <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Marco Slot @ 2024-11-15 11:50 UTC (permalink / raw) To: Aleksander Alekseev <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]> On Fri, Nov 15, 2024 at 9:57 AM Aleksander Alekseev <[email protected]> wrote: > I'm working with the TimescaleDB dev team to fix these issues on the > TimescaleDB side. I looked a bit at this out of interest. I see an assert failure in the lines below when running tests with TimescaleDB compiled against 17.0 with 17.1 installed. Without the assertion it would anyway segfault below. /* * Usually, mt_lastResultIndex matches the target rel. If it happens not * to, we can get the index the hard way with an integer division. */ whichrel = mtstate->mt_lastResultIndex; if (resultRelInfo != mtstate->resultRelInfo + whichrel) { whichrel = resultRelInfo - mtstate->resultRelInfo; Assert(whichrel >= 0 && whichrel < mtstate->mt_nrels); } updateColnos = (List *) list_nth(node->updateColnosLists, whichrel); The problem here is that because TimescaleDB compiled against 17.0 assumes a struct size of 376 (on my laptop) while PostgreSQL allocated the array with a struct size of 384, so the pointer math no longer holds and the whichrel value becomes nonsense. (1736263376 for whatever reason) cheers, Marco ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 12:03 Aleksander Alekseev <[email protected]> parent: Marco Slot <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Aleksander Alekseev @ 2024-11-15 12:03 UTC (permalink / raw) To: pgsql-hackers; +Cc: Marco Slot <[email protected]>; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]> Hi Macro, > I looked a bit at this out of interest. I see an assert failure in the > lines below when running tests with TimescaleDB compiled against 17.0 > with 17.1 installed. Without the assertion it would anyway segfault > below. > > /* > * Usually, mt_lastResultIndex matches the target rel. If it happens not > * to, we can get the index the hard way with an integer division. > */ > whichrel = mtstate->mt_lastResultIndex; > if (resultRelInfo != mtstate->resultRelInfo + whichrel) > { > whichrel = resultRelInfo - mtstate->resultRelInfo; > Assert(whichrel >= 0 && whichrel < mtstate->mt_nrels); > } > > updateColnos = (List *) list_nth(node->updateColnosLists, whichrel); > > The problem here is that because TimescaleDB compiled against 17.0 > assumes a struct size of 376 (on my laptop) while PostgreSQL allocated > the array with a struct size of 384, so the pointer math no longer > holds and the whichrel value becomes nonsense. (1736263376 for > whatever reason) Thanks for reporting. Yes, the code assumed fixed sizeof(ResultRelInfo) within a given PG major release branch which turned out not to be the case. We will investigate whether it can be easily fixed on TimescaleDB side. -- Best regards, Aleksander Alekseev ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 13:21 Mats Kindahl <[email protected]> parent: Noah Misch <[email protected]> 4 siblings, 0 replies; 11+ messages in thread From: Mats Kindahl @ 2024-11-15 13:21 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers On Thu, Nov 14, 2024 at 11:41 PM Noah Misch <[email protected]> wrote: > On Thu, Nov 14, 2024 at 09:33:32PM +0100, Alvaro Herrera wrote: > > On 2024-Nov-14, Tom Lane wrote: > > > Alvaro Herrera <[email protected]> writes: > > > > But timescale did crash: > > > > > > So what is timescale doing differently? > > > src/ts_catalog/catalog.c-extern TSDLLEXPORT ResultRelInfo * > > src/ts_catalog/catalog.c-ts_catalog_open_indexes(Relation heapRel) > > src/ts_catalog/catalog.c-{ > > src/ts_catalog/catalog.c- ResultRelInfo *resultRelInfo; > > src/ts_catalog/catalog.c- > > src/ts_catalog/catalog.c: resultRelInfo = makeNode(ResultRelInfo); > > src/ts_catalog/catalog.c- resultRelInfo->ri_RangeTableIndex = 0; /* > dummy */ > > src/ts_catalog/catalog.c- resultRelInfo->ri_RelationDesc = heapRel; > > src/ts_catalog/catalog.c- resultRelInfo->ri_TrigDesc = NULL; /* we > don't fire triggers */ > > src/ts_catalog/catalog.c- > > src/ts_catalog/catalog.c- ExecOpenIndices(resultRelInfo, false); > > This is a copy of PostgreSQL's CatalogOpenIndexes(). Assuming timescaledb > uses it like PostgreSQL uses CatalogOpenIndexes(), it's fine. > Specifically, > CatalogOpenIndexes() is fine since PostgreSQL does not pass the > ResultRelInfo > to ModifyTable. > This seems to be old code, so I have replaced it with CatalogOpenIndexes (and friends) and it seems to work fine. > > > Oh, hmm, there's this bit which uses struct assignment into a stack > > element: > > Yes, stack allocation of a ResultRelInfo sounds relevant to the crash. It > qualifies as a "very unusual code pattern" in the postgr.es/c/e54a42a > sense. > This is a lesson for us to make sure that we have better checks for these kinds of situations. We should be able to deal with the extension by allocating, e.g., twice the needed memory for the struct (and make sure to zero it out). Then there is no risk of stepping on other object's data. > > > I'm hearing the only confirmed impact on non-assert builds is the need to > recompile timescaledb. (It's unknown whether recompiling will suffice for > timescaledb. For assert builds, six PGXN extensions need recompilation.) I re-compiled and ran with the same version on the server and extension and that seems to work fine. Nothing that stands out. (We have a few other things that behave strange, so might have to take that back.) > I don't see us issuing another set of back branch releases for the purpose > of > making a v16.4-built timescaledb avoid a rebuild. The target audience > would > be someone who can get a new PostgreSQL build but can't get a new > timescaledb > build. That feels like a small audience. What's missing in that analysis? > > Going forward, for struct field additions, I plan to make my own patches > more > ABI-breakage-averse than the postgr.es/c/e54a42a standard. > > > -- Best wishes, Mats Kindahl, Timescale ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 13:27 Mats Kindahl <[email protected]> parent: Marco Slot <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Mats Kindahl @ 2024-11-15 13:27 UTC (permalink / raw) To: Marco Slot <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]> On Fri, Nov 15, 2024 at 12:51 PM Marco Slot <[email protected]> wrote: > On Fri, Nov 15, 2024 at 9:57 AM Aleksander Alekseev > <[email protected]> wrote: > > I'm working with the TimescaleDB dev team to fix these issues on the > > TimescaleDB side. > > I looked a bit at this out of interest. I see an assert failure in the > lines below when running tests with TimescaleDB compiled against 17.0 > with 17.1 installed. Without the assertion it would anyway segfault > below. > > /* > * Usually, mt_lastResultIndex matches the target rel. If it happens > not > * to, we can get the index the hard way with an integer division. > */ > whichrel = mtstate->mt_lastResultIndex; > if (resultRelInfo != mtstate->resultRelInfo + whichrel) > { > whichrel = resultRelInfo - mtstate->resultRelInfo; > Assert(whichrel >= 0 && whichrel < mtstate->mt_nrels); > } > > updateColnos = (List *) list_nth(node->updateColnosLists, whichrel); > > The problem here is that because TimescaleDB compiled against 17.0 > assumes a struct size of 376 (on my laptop) while PostgreSQL allocated > the array with a struct size of 384, so the pointer math no longer > holds and the whichrel value becomes nonsense. (1736263376 for > whatever reason) > That was one of the failures that we had, but we also have a copy of the ExecModifyTable() function and this line would in that case be a problem: 607 608 /* Preload local variables */ 609 resultRelInfo = node->resultRelInfo + node-> mt_lastResultIndex; 610 subplanstate = outerPlanState(node); 611 This stores several resultRelInfo as an array (it seems), which trivially will break if you pass it back and forth between extension and server code (where sizeof(ResultRelInfo) is different). For this case, a List might be better since it will just contain pointers and the extension will in that case just not read the added fields. -- Best wishes, Mats Kindahl, Timescale ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 13:45 Christoph Berg <[email protected]> parent: Noah Misch <[email protected]> 4 siblings, 1 reply; 11+ messages in thread From: Christoph Berg @ 2024-11-15 13:45 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers Re: Noah Misch > I'm hearing the only confirmed impact on non-assert builds is the need to > recompile timescaledb. (It's unknown whether recompiling will suffice for > timescaledb. For assert builds, six PGXN extensions need recompilation.) Which 6 extensions are these? I re-ran the tests on all the extensions on apt.pg.o and didn't see any failures (timescale and age are already rebuilt). Christoph ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 14:32 Noah Misch <[email protected]> parent: Christoph Berg <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Noah Misch @ 2024-11-15 14:32 UTC (permalink / raw) To: Christoph Berg <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Pavan Deolasee <[email protected]>; pgsql-hackers On Fri, Nov 15, 2024 at 02:45:42PM +0100, Christoph Berg wrote: > Re: Noah Misch > > I'm hearing the only confirmed impact on non-assert builds is the need to > > recompile timescaledb. (It's unknown whether recompiling will suffice for > > timescaledb. For assert builds, six PGXN extensions need recompilation.) > > Which 6 extensions are these? > > I re-ran the tests on all the extensions on apt.pg.o and didn't see > any failures (timescale and age are already rebuilt). I counted wrong, it's the five here: https://postgr.es/m/[email protected] They are AGE and the four others you found not to be part of apt.pg.o. (timescaledb is not part of PGXN, so it's not one of the five.) ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-11-15 14:32 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-29 14:19 [PATCH v56 6/6] Exclude pg_stat directory from base backup Kyotaro Horiguchi <[email protected]> 2024-11-14 22:41 Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]> 2024-11-15 00:01 ` Re: Potential ABI breakage in upcoming minor releases Gregory Smith <[email protected]> 2024-11-15 08:56 ` Re: Potential ABI breakage in upcoming minor releases Aleksander Alekseev <[email protected]> 2024-11-15 11:50 ` Re: Potential ABI breakage in upcoming minor releases Marco Slot <[email protected]> 2024-11-15 12:03 ` Re: Potential ABI breakage in upcoming minor releases Aleksander Alekseev <[email protected]> 2024-11-15 13:27 ` Re: Potential ABI breakage in upcoming minor releases Mats Kindahl <[email protected]> 2024-11-15 11:31 ` Re: Potential ABI breakage in upcoming minor releases Alvaro Herrera <[email protected]> 2024-11-15 13:21 ` Re: Potential ABI breakage in upcoming minor releases Mats Kindahl <[email protected]> 2024-11-15 13:45 ` Re: Potential ABI breakage in upcoming minor releases Christoph Berg <[email protected]> 2024-11-15 14:32 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox