public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 03/10] Make sure published XIDs are persistent
6+ messages / 4 participants
[nested] [flat]
* [PATCH 03/10] Make sure published XIDs are persistent
@ 2021-03-08 06:43 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Kyotaro Horiguchi @ 2021-03-08 06:43 UTC (permalink / raw)
pg_xact_status() premises that XIDs obtained by
pg_current_xact_id(_if_assigned)() are persistent beyond a crash. But
XIDs are not guaranteed to go beyond WAL buffers before commit and
thus XIDs may vanish if server crashes before commit. This patch
guarantees the XID shown by the functions to be flushed out to disk.
Copied from: https://www.postgresql.org/message-id/20210308.173242.463790587797836129.horikyota.ntt%40gmail.com
---
src/backend/access/transam/xact.c | 55 +++++++++++++++++++++++++------
src/backend/access/transam/xlog.c | 2 +-
src/backend/utils/adt/xid8funcs.c | 12 ++++++-
src/include/access/xact.h | 3 +-
4 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6395a9b240..38e978d238 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -201,7 +201,7 @@ typedef struct TransactionStateData
int prevSecContext; /* previous SecurityRestrictionContext */
bool prevXactReadOnly; /* entry-time xact r/o state */
bool startedInRecovery; /* did we start in recovery? */
- bool didLogXid; /* has xid been included in WAL record? */
+ XLogRecPtr minLSN; /* LSN needed to reach to record the xid */
int parallelModeLevel; /* Enter/ExitParallelMode counter */
bool chain; /* start a new block after this one */
bool assigned; /* assigned to top-level XID */
@@ -520,14 +520,46 @@ GetCurrentFullTransactionIdIfAny(void)
* MarkCurrentTransactionIdLoggedIfAny
*
* Remember that the current xid - if it is assigned - now has been wal logged.
+ *
+ * upto is the LSN up to which we need to flush WAL to ensure the current xid
+ * to be persistent. See EnsureCurrentTransactionIdLogged().
*/
void
-MarkCurrentTransactionIdLoggedIfAny(void)
+MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto)
{
- if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId))
- CurrentTransactionState->didLogXid = true;
+ if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId) &&
+ XLogRecPtrIsInvalid(CurrentTransactionState->minLSN))
+ CurrentTransactionState->minLSN = upto;
}
+/*
+ * EnsureCurrentTransactionIdLogged
+ *
+ * Make sure that the current top XID is WAL-logged.
+ */
+void
+EnsureTopTransactionIdLogged(void)
+{
+ /*
+ * We need at least one WAL record for the current top transaction to be
+ * flushed out. Write one if we don't have one yet.
+ */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ {
+ xl_xact_assignment xlrec;
+
+ xlrec.xtop = XidFromFullTransactionId(XactTopFullTransactionId);
+ Assert(TransactionIdIsValid(xlrec.xtop));
+ xlrec.nsubxacts = 0;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, MinSizeOfXactAssignment);
+ TopTransactionStateData.minLSN =
+ XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ }
+
+ XLogFlush(TopTransactionStateData.minLSN);
+}
/*
* GetStableLatestTransactionId
@@ -616,14 +648,14 @@ AssignTransactionId(TransactionState s)
* When wal_level=logical, guarantee that a subtransaction's xid can only
* be seen in the WAL stream if its toplevel xid has been logged before.
* If necessary we log an xact_assignment record with fewer than
- * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't set
+ * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if minLSN isn't set
* for a transaction even though it appears in a WAL record, we just might
* superfluously log something. That can happen when an xid is included
* somewhere inside a wal record, but not in XLogRecord->xl_xid, like in
* xl_standby_locks.
*/
if (isSubXact && XLogLogicalInfoActive() &&
- !TopTransactionStateData.didLogXid)
+ XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
log_unknown_top = true;
/*
@@ -693,6 +725,7 @@ AssignTransactionId(TransactionState s)
log_unknown_top)
{
xl_xact_assignment xlrec;
+ XLogRecPtr endptr;
/*
* xtop is always set by now because we recurse up transaction
@@ -707,11 +740,13 @@ AssignTransactionId(TransactionState s)
XLogRegisterData((char *) unreportedXids,
nUnreportedXids * sizeof(TransactionId));
- (void) XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ endptr = XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
nUnreportedXids = 0;
- /* mark top, not current xact as having been logged */
- TopTransactionStateData.didLogXid = true;
+
+ /* set minLSN of top, not of current xact if not yet */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ TopTransactionStateData.minLSN = endptr;
}
}
}
@@ -2022,7 +2057,7 @@ StartTransaction(void)
* initialize reported xid accounting
*/
nUnreportedXids = 0;
- s->didLogXid = false;
+ s->minLSN = InvalidXLogRecPtr;
/*
* must initialize resource-management stuff first
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 04192b7add..0183589b4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1171,7 +1171,7 @@ XLogInsertRecord(XLogRecData *rdata,
*/
WALInsertLockRelease();
- MarkCurrentTransactionIdLoggedIfAny();
+ MarkCurrentTransactionIdLoggedIfAny(EndPos);
END_CRIT_SECTION();
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index cc2b4ac797..992482f8c8 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -357,6 +357,8 @@ bad_format:
Datum
pg_current_xact_id(PG_FUNCTION_ARGS)
{
+ FullTransactionId xid;
+
/*
* Must prevent during recovery because if an xid is not assigned we try
* to assign one, which would fail. Programs already rely on this function
@@ -365,7 +367,12 @@ pg_current_xact_id(PG_FUNCTION_ARGS)
*/
PreventCommandDuringRecovery("pg_current_xact_id()");
- PG_RETURN_FULLTRANSACTIONID(GetTopFullTransactionId());
+ xid = GetTopFullTransactionId();
+
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
+ PG_RETURN_FULLTRANSACTIONID(xid);
}
/*
@@ -380,6 +387,9 @@ pg_current_xact_id_if_assigned(PG_FUNCTION_ARGS)
if (!FullTransactionIdIsValid(topfxid))
PG_RETURN_NULL();
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
PG_RETURN_FULLTRANSACTIONID(topfxid);
}
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 34cfaf542c..a61e4d6da5 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -386,7 +386,8 @@ extern FullTransactionId GetTopFullTransactionId(void);
extern FullTransactionId GetTopFullTransactionIdIfAny(void);
extern FullTransactionId GetCurrentFullTransactionId(void);
extern FullTransactionId GetCurrentFullTransactionIdIfAny(void);
-extern void MarkCurrentTransactionIdLoggedIfAny(void);
+extern void MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto);
+extern void EnsureTopTransactionIdLogged(void);
extern bool SubTransactionIsActive(SubTransactionId subxid);
extern CommandId GetCurrentCommandId(bool used);
extern void SetParallelStartTimestamps(TimestampTz xact_ts, TimestampTz stmt_ts);
--
2.17.0
--0qVF/w3MHQqLSynd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0004-wal_compression_method-default-to-zlib.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: Fix for pageinspect bug in PG 17
@ 2024-11-12 08:04 Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2024-11-12 08:04 UTC (permalink / raw)
To: 'Tomas Vondra' <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Dear Tomas,
> Here's a fix for pageinspect bug in PG17, reported in [1]. The bug turns
> out to be introduced by my commit
I could not see the link, but I think it is [1], right?
>
> commit dae761a87edae444d11a411f711f1d679bed5941
> Author: Tomas Vondra <[email protected]>
> Date: Fri Dec 8 17:07:30 2023 +0100
>
> Add empty BRIN ranges during CREATE INDEX
>
> ...
>
> This adds an out argument to brin_page_items, but I failed to consider
> the user may still run with an older version of the extension - either
> after pg_upgrade (as in the report), or when the CREATE EXTENSION
> command specifies VERSION.
I confirmed that 428c0ca added new output entries (version is bumped 11->12),
and the same C function is used for both 11 and 12.
> The new "empty" field is added in the middle of the output tuple, which
> shifts the values, causing segfault when accessing a text field at the
> end of the array.
Agreed and I could reproduce by steps:
```
postgres=# CREATE EXTENSION pageinspect VERSION "1.11" ;
CREATE EXTENSION
postgres=# CREATE TABLE foo (id int);
CREATE TABLE
postgres=# CREATE INDEX ON foo USING brin ( id );
CREATE INDEX
postgres=# SELECT * FROM brin_page_items(get_raw_page('foo_id_idx', 2), 'foo_id_idx');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
```
>
> Of course, we add arguments to existing functions pretty often, and we
> know how to do that in backwards-compatible way - pg_stat_statements is
> a good example of how to do that nicely. Unfortunately, it's too late to
> do that for brin_page_items :-( There may already be upgraded systems or
> with installed pageinspect, etc.
>
> The only fix I can think of is explicitly looking at TupleDesc->natts,
> as in the attached fix.
I've tested your patch and it worked well. Also, I tried to upgrade from 11 and 12
and ran again, it could execute well.
Few points:
1.
Do you think we should follow the approach like pg_stat_statements for master?
Or, do you want to apply the patch both PG17 and HEAD? I think latter one is OK
because we should avoid code branches as much as possible.
2.
Later readers may surprise the part for checking `rsinfo->setDesc->natts`.
Can you use the boolean and add comments, something like
```
+ /* Support older API version */
+ bool output_empty_attr = (rsinfo->setDesc->natts >= 8);
```
3.
Do we have to add the test for the fix?
[1]: https://www.postgresql.org/message-id/VI1PR02MB63331C3D90E2104FD12399D38A5D2%40VI1PR02MB6333.eurprd0...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix for pageinspect bug in PG 17
@ 2024-11-13 16:06 Tomas Vondra <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tomas Vondra @ 2024-11-13 16:06 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 11/12/24 09:04, Hayato Kuroda (Fujitsu) wrote:
> Dear Tomas,
>
>> Here's a fix for pageinspect bug in PG17, reported in [1]. The bug turns
>> out to be introduced by my commit
>
> I could not see the link, but I think it is [1], right?
>
Right. Apologies, I forgot to include the link.
>>
>> commit dae761a87edae444d11a411f711f1d679bed5941
>> Author: Tomas Vondra <[email protected]>
>> Date: Fri Dec 8 17:07:30 2023 +0100
>>
>> Add empty BRIN ranges during CREATE INDEX
>>
>> ...
>>
>> This adds an out argument to brin_page_items, but I failed to consider
>> the user may still run with an older version of the extension - either
>> after pg_upgrade (as in the report), or when the CREATE EXTENSION
>> command specifies VERSION.
>
> I confirmed that 428c0ca added new output entries (version is bumped 11->12),
> and the same C function is used for both 11 and 12.
>
>> The new "empty" field is added in the middle of the output tuple, which
>> shifts the values, causing segfault when accessing a text field at the
>> end of the array.
>
> Agreed and I could reproduce by steps:
>
> ```
> postgres=# CREATE EXTENSION pageinspect VERSION "1.11" ;
> CREATE EXTENSION
> postgres=# CREATE TABLE foo (id int);
> CREATE TABLE
> postgres=# CREATE INDEX ON foo USING brin ( id );
> CREATE INDEX
> postgres=# SELECT * FROM brin_page_items(get_raw_page('foo_id_idx', 2), 'foo_id_idx');
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> ```
>
>>
>> Of course, we add arguments to existing functions pretty often, and we
>> know how to do that in backwards-compatible way - pg_stat_statements is
>> a good example of how to do that nicely. Unfortunately, it's too late to
>> do that for brin_page_items :-( There may already be upgraded systems or
>> with installed pageinspect, etc.
>>
>> The only fix I can think of is explicitly looking at TupleDesc->natts,
>> as in the attached fix.
>
> I've tested your patch and it worked well. Also, I tried to upgrade from 11 and 12
> and ran again, it could execute well.
>
> Few points:
>
> 1.
> Do you think we should follow the approach like pg_stat_statements for master?
> Or, do you want to apply the patch both PG17 and HEAD? I think latter one is OK
> because we should avoid code branches as much as possible.
>
My plan was to apply the patch to both 17 and HEAD, and then maybe do
something smarter in HEAD in a separate commit. But then Michael pointed
out other pageinspect functions just error out in this version-mismatch
cases, so I think it's better to just do it the same way.
Maybe we could be smarter, but it seems pointless to do it only for one
pageinspect function, while still requiring an upgrade for other more
widely used functions in the same situation (albeit for a much older
version of the extension).
> 2.
> Later readers may surprise the part for checking `rsinfo->setDesc->natts`.
> Can you use the boolean and add comments, something like
>
> ```
> + /* Support older API version */
> + bool output_empty_attr = (rsinfo->setDesc->natts >= 8);
> ```
>
Doesn't matter, if we error out.
> 3.
> Do we have to add the test for the fix?
>
Yes. See the patch I shared a couple minutes ago.
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: Fix for pageinspect bug in PG 17
@ 2024-11-14 02:56 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2024-11-14 02:56 UTC (permalink / raw)
To: 'Tomas Vondra' <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Dear Tomas,
Thanks for updating the patch.
I've tested new patch and confirmed the brin_pgage_items() could error out:
```
postgres=# SELECT * FROM brin_page_items(get_raw_page('foo_id_idx', 2), 'foo_id_idx');
ERROR: function has wrong number of declared columns
HINT: To resolve the problem, update the "pageinspect" extension to the latest version.
```
I also do not have strong opinions for both approaches.
If I had to say... it's OK to do error out. The source becomes simpler and I cannot
find use-case that user strongly wants to use the older pageinspect extension.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix for pageinspect bug in PG 17
@ 2024-12-17 17:01 Tomas Vondra <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tomas Vondra @ 2024-12-17 17:01 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
I've pushed (and backpatched) a fix for this.
I ended up doing the simplest thing -- error out if the number of
columns does not match, suggesting to update to latest extension version.
I considered handling it in a nicer way, but I didn't like the result
very much and I think that's sufficient for superuser-only extension.
And 691e8b2e18 seems like a reasonable precedent (even though the
backbranches did do a different thing).
I also considered introducing pg_stat_statements-style versioning, but
it's too late to do that in backbranches, and I don't think we expect
the function to change very often to justify this.
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Fix for pageinspect bug in PG 17
@ 2024-12-19 05:49 Michael Paquier <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2024-12-19 05:49 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Dec 17, 2024 at 06:01:39PM +0100, Tomas Vondra wrote:
> I also considered introducing pg_stat_statements-style versioning, but
> it's too late to do that in backbranches, and I don't think we expect
> the function to change very often to justify this.
Sounds good to me, thanks!
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-12-19 05:49 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 06:43 [PATCH 03/10] Make sure published XIDs are persistent Kyotaro Horiguchi <[email protected]>
2024-11-12 08:04 RE: Fix for pageinspect bug in PG 17 Hayato Kuroda (Fujitsu) <[email protected]>
2024-11-13 16:06 ` Re: Fix for pageinspect bug in PG 17 Tomas Vondra <[email protected]>
2024-11-14 02:56 ` RE: Fix for pageinspect bug in PG 17 Hayato Kuroda (Fujitsu) <[email protected]>
2024-12-17 17:01 ` Re: Fix for pageinspect bug in PG 17 Tomas Vondra <[email protected]>
2024-12-19 05:49 ` Re: Fix for pageinspect bug in PG 17 Michael Paquier <[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