public inbox for [email protected]
help / color / mirror / Atom feedBUG #15449: file_fdw using program cause exit code error when using LIMIT
15+ messages / 8 participants
[nested] [flat]
* BUG #15449: file_fdw using program cause exit code error when using LIMIT
@ 2018-10-22 14:00 PG Bug reporting form <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: PG Bug reporting form @ 2018-10-22 14:00 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
The following bug has been logged on the website:
Bug reference: 15449
Logged by: Eric Cyr
Email address: [email protected]
PostgreSQL version: 10.5
Operating system: Ubuntu 16.04.2 LTS (Xenial Xerus)
Description:
CREATE FOREIGN TABLE test_file_fdw_program_limit
(
message text
)
SERVER pg_log_fdw OPTIONS (program 'echo "test"', format 'csv')
;
--
SELECT * FROM test_file_fdw_program_limit;
/*
OK
*/
SELECT * FROM test_file_fdw_program_limit LIMIT 2;
/*
OK
*/
SELECT * FROM test_file_fdw_program_limit LIMIT 0;
/*
[38000] ERROR: program "echo "test"" failed Detail: child process exited
with exit code 1
*/
--
LIMIT 0 is the easiest way we found to reproduce the error, but we
encountered same issue with LIMIT (< file lines) on a program doing an if
exists + file output stream manipulation.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT
@ 2018-10-22 17:27 Tom Lane <[email protected]>
parent: PG Bug reporting form <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2018-10-22 17:27 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
=?utf-8?q?PG_Bug_reporting_form?= <[email protected]> writes:
> SELECT * FROM test_file_fdw_program_limit LIMIT 0;
> /*
> [38000] ERROR: program "echo "test"" failed Detail: child process exited
> with exit code 1
> */
> LIMIT 0 is the easiest way we found to reproduce the error, but we
> encountered same issue with LIMIT (< file lines) on a program doing an if
> exists + file output stream manipulation.
Yeah, I can reproduce this on macOS as well as Linux. Capturing stderr
shows something pretty unsurprising:
sh: line 1: echo: write error: Broken pipe
I get similar behavior from "cat"; didn't try anything else.
So the called program is behaving in a somewhat reasonable way: it's
detecting EPIPE on its stdout (after we close the pipe), reporting that,
and doing exit(1).
Unfortunately, it's not clear what we could do about that, short of
always reading the whole program output, which is likely to be a
cure worse than the disease. If the program were failing thanks to
SIGPIPE, we could recognize that as a case we can ignore ... but with
behavior like this, I don't see a reliable way to tell it apart from
a generic program failure, which surely we'd better report.
The only idea that comes to mind is to introduce a file_fdw option
specifying "read program input to end, even if query doesn't want
it all". Not quite sure which choice should be the default; you
could make an argument either way I think.
Curiously, if you do stuff like "cat longfile | head -10", you don't
get any visible error, though surely cat must be seeing the same
thing underneath ... I wonder how the shell is arranging that?
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT
@ 2018-10-23 12:21 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2018-10-23 12:21 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]
I wrote:
> =?utf-8?q?PG_Bug_reporting_form?= <[email protected]> writes:
>> SELECT * FROM test_file_fdw_program_limit LIMIT 0;
>> /*
>> [38000] ERROR: program "echo "test"" failed Detail: child process exited
>> with exit code 1
>> */
> Yeah, I can reproduce this on macOS as well as Linux. Capturing stderr
> shows something pretty unsurprising:
> sh: line 1: echo: write error: Broken pipe
> So the called program is behaving in a somewhat reasonable way: it's
> detecting EPIPE on its stdout (after we close the pipe), reporting that,
> and doing exit(1).
> Unfortunately, it's not clear what we could do about that, short of
> always reading the whole program output, which is likely to be a
> cure worse than the disease. If the program were failing thanks to
> SIGPIPE, we could recognize that as a case we can ignore ... but with
> behavior like this, I don't see a reliable way to tell it apart from
> a generic program failure, which surely we'd better report.
After a bit of thought, the problem here is blindingly obvious:
we generally run the backend with SIGPIPE handing set to SIG_IGN,
and evidently popen() allows the called program to inherit that,
at least on these platforms.
So what we need to do is make sure the called program inherits SIG_DFL
handling for SIGPIPE, and then special-case that result as not being
a failure. The attached POC patch does that and successfully makes
the file_fdw problem go away for me.
It's just a POC because there are some things that need more thought
than I've given them:
1. Is it OK to revert SIGPIPE to default processing for *all* programs
launched through OpenPipeStream? If not, what infrastructure do we
need to add to control that? In particular, is it sane to revert
SIGPIPE for a pipe program that we will write to not read from?
(I thought probably yes, because that is the normal Unix behavior,
but it could be debated.)
2. Likewise, is it always OK for ClosePipeToProgram to ignore a
SIGPIPE failure? (For ordinary COPY, maybe it shouldn't, since
we don't intend to terminate that early.)
3. Maybe this should be implemented at some higher level?
4. Are there any other signals we ought to be reverting to default
behavior before launching a COPY TO/FROM PROGRAM?
regards, tom lane
Attachments:
[text/x-patch] process-sigpipe-normally-in-copy-from-program-1.patch (2.7K, ../../[email protected]/2-process-sigpipe-normally-in-copy-from-program-1.patch)
download | inline diff:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index b58a74f4e3db9d8e8f30d35e08ca84814742faa3..c4c6c875ec8102df2756d8ea1ba79791d63fed25 100644
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*************** ClosePipeToProgram(CopyState cstate)
*** 1727,1737 ****
--- 1727,1746 ----
(errcode_for_file_access(),
errmsg("could not close pipe to external command: %m")));
else if (pclose_rc != 0)
+ {
+ /*
+ * If the called program terminated on SIGPIPE, assume it's OK; we
+ * must have chosen to stop reading its output early.
+ */
+ if (WIFSIGNALED(pclose_rc) && WTERMSIG(pclose_rc) == SIGPIPE)
+ return;
+
ereport(ERROR,
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg("program \"%s\" failed",
cstate->filename),
errdetail_internal("%s", wait_result_to_str(pclose_rc))));
+ }
}
/*
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 8dd51f176749c5d1b2adc9993b5a9a3571ee566c..6cec85e2c1dc7dea23c8b941e5080714ea65e57a 100644
*** a/src/backend/storage/file/fd.c
--- b/src/backend/storage/file/fd.c
*************** OpenTransientFilePerm(const char *fileNa
*** 2430,2440 ****
--- 2430,2445 ----
* Routines that want to initiate a pipe stream should use OpenPipeStream
* rather than plain popen(). This lets fd.c deal with freeing FDs if
* necessary. When done, call ClosePipeStream rather than pclose.
+ *
+ * This function also ensures that the popen'd program is run with default
+ * SIGPIPE processing, rather than the SIG_IGN setting the backend normally
+ * uses. This ensures desirable response to, eg, closing a read pipe early.
*/
FILE *
OpenPipeStream(const char *command, const char *mode)
{
FILE *file;
+ int save_errno;
DO_DB(elog(LOG, "OpenPipeStream: Allocated %d (%s)",
numAllocatedDescs, command));
*************** OpenPipeStream(const char *command, cons
*** 2452,2459 ****
TryAgain:
fflush(stdout);
fflush(stderr);
errno = 0;
! if ((file = popen(command, mode)) != NULL)
{
AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];
--- 2457,2469 ----
TryAgain:
fflush(stdout);
fflush(stderr);
+ pqsignal(SIGPIPE, SIG_DFL);
errno = 0;
! file = popen(command, mode);
! save_errno = errno;
! pqsignal(SIGPIPE, SIG_IGN);
! errno = save_errno;
! if (file != NULL)
{
AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];
*************** TryAgain:
*** 2466,2473 ****
if (errno == EMFILE || errno == ENFILE)
{
- int save_errno = errno;
-
ereport(LOG,
(errcode(ERRCODE_INSUFFICIENT_RESOURCES),
errmsg("out of file descriptors: %m; release and retry")));
--- 2476,2481 ----
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT
@ 2018-10-29 06:58 Kyotaro HORIGUCHI <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Kyotaro HORIGUCHI @ 2018-10-29 06:58 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]
Hello.
At Tue, 23 Oct 2018 13:21:31 +0100, Tom Lane <[email protected]> wrote in <[email protected]>
> I wrote:
> > =?utf-8?q?PG_Bug_reporting_form?= <[email protected]> writes:
> >> SELECT * FROM test_file_fdw_program_limit LIMIT 0;
> >> /*
> >> [38000] ERROR: program "echo "test"" failed Detail: child process exited
> >> with exit code 1
> >> */
>
> > Yeah, I can reproduce this on macOS as well as Linux. Capturing stderr
> > shows something pretty unsurprising:
> > sh: line 1: echo: write error: Broken pipe
> > So the called program is behaving in a somewhat reasonable way: it's
> > detecting EPIPE on its stdout (after we close the pipe), reporting that,
> > and doing exit(1).
> > Unfortunately, it's not clear what we could do about that, short of
> > always reading the whole program output, which is likely to be a
> > cure worse than the disease. If the program were failing thanks to
> > SIGPIPE, we could recognize that as a case we can ignore ... but with
> > behavior like this, I don't see a reliable way to tell it apart from
> > a generic program failure, which surely we'd better report.
>
> After a bit of thought, the problem here is blindingly obvious:
> we generally run the backend with SIGPIPE handing set to SIG_IGN,
> and evidently popen() allows the called program to inherit that,
> at least on these platforms.
>
> So what we need to do is make sure the called program inherits SIG_DFL
> handling for SIGPIPE, and then special-case that result as not being
> a failure. The attached POC patch does that and successfully makes
> the file_fdw problem go away for me.
>
> It's just a POC because there are some things that need more thought
> than I've given them:
>
> 1. Is it OK to revert SIGPIPE to default processing for *all* programs
> launched through OpenPipeStream? If not, what infrastructure do we
> need to add to control that? In particular, is it sane to revert
> SIGPIPE for a pipe program that we will write to not read from?
> (I thought probably yes, because that is the normal Unix behavior,
> but it could be debated.)
Forcing to ignore SIGPIPE lets the program to handle EPIPE, which
might not be handled poperly since it would be unexpected by the
program.
If we don't read from the pipe (that is, we are writing to it),
anyway we shouldn't change the behavior since SIGPIPE can come
from another pipe.
> 2. Likewise, is it always OK for ClosePipeToProgram to ignore a
> SIGPIPE failure? (For ordinary COPY, maybe it shouldn't, since
> we don't intend to terminate that early.)
Since the SIGPIPE may come from another pipe, I think we
shouldn't generally. But iff we are explicitly stop reading from
the pipe before detecting an error, it can be ignored since we
aren't interested in further failure. Thus ClosePipeToProgram
might need an extra parameter or CopyState may need an additional
flag named, say, "explicit_close" (or
"ignore_sigpipe_on_pclose"..) in CopyState which will be set in
EndCopy.
> 3. Maybe this should be implemented at some higher level?
I'm not sure, but it seems to me not needed.
> 4. Are there any other signals we ought to be reverting to default
> behavior before launching a COPY TO/FROM PROGRAM?
All handlers other than SIG_DEF and IGN are reset on exec().
Only SIGUSR2 seems to be set to SIG_IGN, but it's not likely to
harm anything. Perhaps it would be safer against future changes
if we explicitly reset all changed actions to SIG_DEF, but it
might be too-much..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-02-15 00:56 Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Kyotaro Horiguchi @ 2023-02-15 00:56 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]
At Tue, 14 Feb 2023 15:43:26 +0100, "Drouvot, Bertrand" <[email protected]> wrote in
> Oh right, my bad (the issue has been introduced in V2).
> Fixed in V4.
Great!
> > I thought that we might be able to return entry_ref->pending since the
> > callers don't call pfree on the returned pointer, but it is not great
> > that we don't inform the callers if the returned memory can be safely
> > pfreed or not.
> > Thus what I have in mind is the following.
> >
> >> if (!entry_ref)
> >> + {
> >> entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
> >> InvalidOid, rel_id);
> >> + if (!entry_ref)
> >> + return NULL;
> >> + }
>
> LGTM, done that way in V4.
That part looks good to me, thanks!
I was going through v4 and it seems to me that the comment for
find_tabstat_entry may not be quite right.
> * find any existing PgStat_TableStatus entry for rel
> *
> * Find any existing PgStat_TableStatus entry for rel_id in the current
> * database. If not found, try finding from shared tables.
> *
> * If no entry found, return NULL, don't create a new one
The comment assumed that the function directly returns an entry from
shared memory, but now it copies the entry's contents into a palloc'ed
memory and stores the sums of some counters for the current
transaction in it. Do you think we should update the comment to
reflect this change?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-02-15 08:21 Drouvot, Bertrand <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Drouvot, Bertrand @ 2023-02-15 08:21 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]
Hi,
On 2/15/23 1:56 AM, Kyotaro Horiguchi wrote:
> At Tue, 14 Feb 2023 15:43:26 +0100, "Drouvot, Bertrand" <[email protected]> wrote in
>
> The comment assumed that the function directly returns an entry from
> shared memory, but now it copies the entry's contents into a palloc'ed
> memory and stores the sums of some counters for the current
> transaction in it. Do you think we should update the comment to
> reflect this change?
>
Good point, thanks! Yeah, definitively, done in V5 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index f793ac1516..b26e2a5a7a 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. There is no need for the
+ * caller to pfree the copy as the MemoryContext will be reset soon after.
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in t_counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
+ tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
+ tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9d707c3521..405d080daa 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1548,17 +1548,11 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_inserted;
- /* live subtransactions' counts aren't in t_tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_inserted);
PG_RETURN_INT64(result);
}
@@ -1569,17 +1563,11 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_updated;
- /* live subtransactions' counts aren't in t_tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_updated);
PG_RETURN_INT64(result);
}
@@ -1590,17 +1578,11 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_deleted;
- /* live subtransactions' counts aren't in t_tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_deleted);
PG_RETURN_INT64(result);
}
Attachments:
[text/plain] v5-0001-find_tabstat_entry_recon.patch (3.9K, ../../[email protected]/2-v5-0001-find_tabstat_entry_recon.patch)
download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index f793ac1516..b26e2a5a7a 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. There is no need for the
+ * caller to pfree the copy as the MemoryContext will be reset soon after.
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in t_counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
+ tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
+ tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9d707c3521..405d080daa 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1548,17 +1548,11 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_inserted;
- /* live subtransactions' counts aren't in t_tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_inserted);
PG_RETURN_INT64(result);
}
@@ -1569,17 +1563,11 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_updated;
- /* live subtransactions' counts aren't in t_tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_updated);
PG_RETURN_INT64(result);
}
@@ -1590,17 +1578,11 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_deleted;
- /* live subtransactions' counts aren't in t_tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_deleted);
PG_RETURN_INT64(result);
}
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-02-16 21:21 Andres Freund <[email protected]>
parent: Drouvot, Bertrand <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2023-02-16 21:21 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 2023-02-15 09:21:48 +0100, Drouvot, Bertrand wrote:
> diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
> index f793ac1516..b26e2a5a7a 100644
> --- a/src/backend/utils/activity/pgstat_relation.c
> +++ b/src/backend/utils/activity/pgstat_relation.c
> @@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
> * Find any existing PgStat_TableStatus entry for rel_id in the current
> * database. If not found, try finding from shared tables.
> *
> + * If an entry is found, copy it and increment the copy's counters with their
> + * subtransactions counterparts. Then return the copy. There is no need for the
> + * caller to pfree the copy as the MemoryContext will be reset soon after.
> + *
The "There is no need" bit seems a bit off. Yes, that's true for the current
callers, but who says that it has to stay that way?
Otherwise this looks ready, on a casual scan.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-03-06 07:33 Drouvot, Bertrand <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Drouvot, Bertrand @ 2023-03-06 07:33 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 2/16/23 10:21 PM, Andres Freund wrote:
> Hi,
>
> On 2023-02-15 09:21:48 +0100, Drouvot, Bertrand wrote:
>> diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
>> index f793ac1516..b26e2a5a7a 100644
>> --- a/src/backend/utils/activity/pgstat_relation.c
>> +++ b/src/backend/utils/activity/pgstat_relation.c
>> @@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
>> * Find any existing PgStat_TableStatus entry for rel_id in the current
>> * database. If not found, try finding from shared tables.
>> *
>> + * If an entry is found, copy it and increment the copy's counters with their
>> + * subtransactions counterparts. Then return the copy. There is no need for the
>> + * caller to pfree the copy as the MemoryContext will be reset soon after.
>> + *
>
> The "There is no need" bit seems a bit off. Yes, that's true for the current
> callers, but who says that it has to stay that way?
>
Good point. Wording has been changed in V6 attached.
> Otherwise this looks ready, on a casual scan.
>
Thanks for having looked at it!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index f793ac1516..c030e1782e 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in t_counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
+ tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
+ tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index b61a12382b..b4de57c535 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1546,17 +1546,11 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_inserted;
- /* live subtransactions' counts aren't in t_tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_inserted);
PG_RETURN_INT64(result);
}
@@ -1567,17 +1561,11 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_updated;
- /* live subtransactions' counts aren't in t_tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_updated);
PG_RETURN_INT64(result);
}
@@ -1588,17 +1576,11 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_deleted;
- /* live subtransactions' counts aren't in t_tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_deleted);
PG_RETURN_INT64(result);
}
Attachments:
[text/plain] v6-0001-find_tabstat_entry_recon.patch (3.9K, ../../[email protected]/2-v6-0001-find_tabstat_entry_recon.patch)
download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index f793ac1516..c030e1782e 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -471,20 +471,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* Find any existing PgStat_TableStatus entry for rel_id in the current
* database. If not found, try finding from shared tables.
*
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ PgStat_TableXactStatus *trans;
+ PgStat_TableStatus *tabentry = NULL;
+ PgStat_TableStatus *tablestatus = NULL;
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
if (!entry_ref)
+ {
entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+ if(!entry_ref)
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /*
+ * Live subtransactions' counts aren't in t_counts yet. This is not a hot
+ * code path so it sounds ok to reconcile for tuples_inserted,
+ * tuples_updated and tuples_deleted even if this is not what the caller
+ * is interested in.
+ */
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
+ tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
+ tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index b61a12382b..b4de57c535 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1546,17 +1546,11 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_inserted;
- /* live subtransactions' counts aren't in t_tuples_inserted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_inserted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_inserted);
PG_RETURN_INT64(result);
}
@@ -1567,17 +1561,11 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_updated;
- /* live subtransactions' counts aren't in t_tuples_updated yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_updated;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_updated);
PG_RETURN_INT64(result);
}
@@ -1588,17 +1576,11 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
Oid relid = PG_GETARG_OID(0);
int64 result;
PgStat_TableStatus *tabentry;
- PgStat_TableXactStatus *trans;
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
- {
- result = tabentry->t_counts.t_tuples_deleted;
- /* live subtransactions' counts aren't in t_tuples_deleted yet */
- for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
- result += trans->tuples_deleted;
- }
+ result = (int64) (tabentry->t_counts.t_tuples_deleted);
PG_RETURN_INT64(result);
}
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
@ 2023-03-16 06:29 Michael Paquier <[email protected]>
parent: Drouvot, Bertrand <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Michael Paquier @ 2023-03-16 06:29 UTC (permalink / raw)
To: Drouvot, Bertrand <[email protected]>; +Cc: Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
On Mon, Mar 06, 2023 at 08:33:15AM +0100, Drouvot, Bertrand wrote:
> Thanks for having looked at it!
Looking at that, I have a few comments.
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
[...]
+ for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+ {
+ tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
+ tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
+ tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
+ }
- if (entry_ref)
- return entry_ref->pending;
- return NULL;
+ return tablestatus;
From what I get with this change, the number of tuples changed by DMLs
have their computations done a bit earlier, meaning that it would make
all the callers of find_tabstat_entry() pay the computation cost.
Still it is not really going to matter, because we will just do the
computation once when looking at any pending changes of
pg_stat_xact_all_tables for each entry. There are 9 callers of
find_tabstat_entry, with 7 being used for pg_stat_xact_all_tables.
How much do we need to care about the remaining two callers
pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()?
Could it be a problem if these two also pay the extra computation cost
if a transaction with many subtransactions (aka )needs to look at their
data? These two are used nowhere, they have pg_proc entries and they
are undocumented, so it is hard to say the impact of this change on
them..
Second question: once the data from the subtransactions is copied,
would it be cleaner to set trans to NULL after the data copy is done?
It would feel a bit safer to me to document that find_tabstat_entry()
is currently only used for this xact system view.. The extra
computation could lead to surprises, actually, if this routine is used
outside this context? Perhaps that's OK, but it does not give me a
warm feeling, just to reshape three functions of pgstatfuncs.c with
macros.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 79 +++++------------------------
src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
src/include/access/heapam.h | 31 ++++++-----
3 files changed, 77 insertions(+), 84 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
ItemPointer ctid, TransactionId xid,
LockTupleMode mode);
-static int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
- xl_heap_freeze_plan *plans_out,
- OffsetNumber *offsets_out);
static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
uint16 *new_infomask2);
static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
htup = (HeapTupleHeader) PageGetItem(page, itemid);
heap_execute_freeze_tuple(htup, frz);
}
-
- MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
- OffsetNumber offsets[MaxHeapTuplesPerPage];
- int nplans;
- xl_heap_freeze_page xlrec;
- XLogRecPtr recptr;
-
- /* Prepare deduplicated representation for use in WAL record */
- nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
- xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
- xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
- xlrec.nplans = nplans;
-
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
- /*
- * The freeze plan array and offset array are not actually in the
- * buffer, but pretend that they are. When XLogInsert stores the
- * whole buffer, the arrays need not be stored too.
- */
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
- XLogRegisterBufData(0, (char *) plans,
- nplans * sizeof(xl_heap_freeze_plan));
- XLogRegisterBufData(0, (char *) offsets,
- ntuples * sizeof(OffsetNumber));
-
- recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
- PageSetLSN(page, recptr);
- }
-
- END_CRIT_SECTION();
}
/*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
* (actually there is one array per freeze plan, but that's not of immediate
* concern to our caller).
*/
-static int
+int
heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
xl_heap_freeze_plan *plans_out,
OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- presult->frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ {
+ xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
+ int nplans;
+ xl_heap_freeze_page xlrec;
+ XLogRecPtr recptr;
+
+ /*
+ * Prepare deduplicated representation for use in WAL record
+ * Destructively sorts tuples array in-place.
+ */
+ nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+ xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+ xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+ xlrec.nplans = nplans;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+ /*
+ * The freeze plan array and offset array are not actually in the
+ * buffer, but pretend that they are. When XLogInsert stores the
+ * whole buffer, the arrays need not be stored too.
+ */
+ XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+ XLogRegisterBufData(0, (char *) plans,
+ nplans * sizeof(xl_heap_freeze_plan));
+ XLogRegisterBufData(0, (char *) offsets,
+ presult->nfrozen * sizeof(OffsetNumber));
+
+ recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+ PageSetLSN(page, recptr);
+ }
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+ xl_heap_freeze_plan *plans_out,
+ OffsetNumber *offsets_out);
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0013-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 79 +++++------------------------
src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
src/include/access/heapam.h | 31 ++++++-----
3 files changed, 77 insertions(+), 84 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
ItemPointer ctid, TransactionId xid,
LockTupleMode mode);
-static int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
- xl_heap_freeze_plan *plans_out,
- OffsetNumber *offsets_out);
static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
uint16 *new_infomask2);
static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
htup = (HeapTupleHeader) PageGetItem(page, itemid);
heap_execute_freeze_tuple(htup, frz);
}
-
- MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
- OffsetNumber offsets[MaxHeapTuplesPerPage];
- int nplans;
- xl_heap_freeze_page xlrec;
- XLogRecPtr recptr;
-
- /* Prepare deduplicated representation for use in WAL record */
- nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
- xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
- xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
- xlrec.nplans = nplans;
-
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
- /*
- * The freeze plan array and offset array are not actually in the
- * buffer, but pretend that they are. When XLogInsert stores the
- * whole buffer, the arrays need not be stored too.
- */
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
- XLogRegisterBufData(0, (char *) plans,
- nplans * sizeof(xl_heap_freeze_plan));
- XLogRegisterBufData(0, (char *) offsets,
- ntuples * sizeof(OffsetNumber));
-
- recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
- PageSetLSN(page, recptr);
- }
-
- END_CRIT_SECTION();
}
/*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
* (actually there is one array per freeze plan, but that's not of immediate
* concern to our caller).
*/
-static int
+int
heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
xl_heap_freeze_plan *plans_out,
OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- presult->frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ {
+ xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
+ int nplans;
+ xl_heap_freeze_page xlrec;
+ XLogRecPtr recptr;
+
+ /*
+ * Prepare deduplicated representation for use in WAL record
+ * Destructively sorts tuples array in-place.
+ */
+ nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+ xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+ xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+ xlrec.nplans = nplans;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+ /*
+ * The freeze plan array and offset array are not actually in the
+ * buffer, but pretend that they are. When XLogInsert stores the
+ * whole buffer, the arrays need not be stored too.
+ */
+ XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+ XLogRegisterBufData(0, (char *) plans,
+ nplans * sizeof(xl_heap_freeze_plan));
+ XLogRegisterBufData(0, (char *) offsets,
+ presult->nfrozen * sizeof(OffsetNumber));
+
+ recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+ PageSetLSN(page, recptr);
+ }
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+ xl_heap_freeze_plan *plans_out,
+ OffsetNumber *offsets_out);
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0013-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v4 12/19] Remove heap_freeze_execute_prepared()
@ 2024-01-07 22:03 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-01-07 22:03 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 79 +++++------------------------
src/backend/access/heap/pruneheap.c | 51 +++++++++++++++++--
src/include/access/heapam.h | 31 ++++++-----
3 files changed, 77 insertions(+), 84 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 16e3f2520a4..e47b56e7856 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -91,9 +91,6 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple,
ItemPointer ctid, TransactionId xid,
LockTupleMode mode);
-static int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
- xl_heap_freeze_plan *plans_out,
- OffsetNumber *offsets_out);
static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
uint16 *new_infomask2);
static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax,
@@ -6343,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6659,8 +6656,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6713,30 +6710,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6746,45 +6730,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
htup = (HeapTupleHeader) PageGetItem(page, itemid);
heap_execute_freeze_tuple(htup, frz);
}
-
- MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
- OffsetNumber offsets[MaxHeapTuplesPerPage];
- int nplans;
- xl_heap_freeze_page xlrec;
- XLogRecPtr recptr;
-
- /* Prepare deduplicated representation for use in WAL record */
- nplans = heap_log_freeze_plan(tuples, ntuples, plans, offsets);
-
- xlrec.snapshotConflictHorizon = snapshotConflictHorizon;
- xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(rel);
- xlrec.nplans = nplans;
-
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
-
- /*
- * The freeze plan array and offset array are not actually in the
- * buffer, but pretend that they are. When XLogInsert stores the
- * whole buffer, the arrays need not be stored too.
- */
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
- XLogRegisterBufData(0, (char *) plans,
- nplans * sizeof(xl_heap_freeze_plan));
- XLogRegisterBufData(0, (char *) offsets,
- ntuples * sizeof(OffsetNumber));
-
- recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
-
- PageSetLSN(page, recptr);
- }
-
- END_CRIT_SECTION();
}
/*
@@ -6874,7 +6819,7 @@ heap_log_freeze_new_plan(xl_heap_freeze_plan *plan, HeapTupleFreeze *frz)
* (actually there is one array per freeze plan, but that's not of immediate
* concern to our caller).
*/
-static int
+int
heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
xl_heap_freeze_plan *plans_out,
OffsetNumber *offsets_out)
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 87f99497865..7bd479cfd4e 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -640,10 +640,53 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- presult->frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ {
+ xl_heap_freeze_plan plans[MaxHeapTuplesPerPage];
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
+ int nplans;
+ xl_heap_freeze_page xlrec;
+ XLogRecPtr recptr;
+
+ /*
+ * Prepare deduplicated representation for use in WAL record
+ * Destructively sorts tuples array in-place.
+ */
+ nplans = heap_log_freeze_plan(prstate.frozen, presult->nfrozen, plans, offsets);
+
+ xlrec.snapshotConflictHorizon = presult->frz_conflict_horizon;
+ xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation);
+ xlrec.nplans = nplans;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage);
+
+ /*
+ * The freeze plan array and offset array are not actually in the
+ * buffer, but pretend that they are. When XLogInsert stores the
+ * whole buffer, the arrays need not be stored too.
+ */
+ XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+ XLogRegisterBufData(0, (char *) plans,
+ nplans * sizeof(xl_heap_freeze_plan));
+ XLogRegisterBufData(0, (char *) offsets,
+ presult->nfrozen * sizeof(OffsetNumber));
+
+ recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE);
+
+ PageSetLSN(page, recptr);
+ }
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 02e33f213e1..321a46185e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -315,12 +316,16 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
+extern int heap_log_freeze_plan(HeapTupleFreeze *tuples, int ntuples,
+ xl_heap_freeze_plan *plans_out,
+ OffsetNumber *offsets_out);
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0013-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v9 11/21] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 49 +++++++----------------------
src/backend/access/heap/pruneheap.c | 22 ++++++++++---
src/include/access/heapam.h | 28 +++++++++--------
3 files changed, 44 insertions(+), 55 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b3119de2aa6..41c1c7d286f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6445,9 +6445,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6762,8 +6762,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6816,30 +6816,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6851,20 +6838,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
}
MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
- false, /* no cleanup lock required */
- PRUNE_VACUUM_SCAN,
- tuples, ntuples,
- NULL, 0, /* redirected */
- NULL, 0, /* dead */
- NULL, 0); /* unused */
- }
-
- END_CRIT_SECTION();
}
/*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 13db348b2c1..5d8c881c2fc 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ log_heap_prune_and_freeze(relation, buffer,
+ frz_conflict_horizon, false, reason,
+ prstate.frozen, presult->nfrozen,
+ NULL, 0, /* redirected */
+ NULL, 0, /* dead */
+ NULL, 0); /* unused */
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index cc3b3346bc4..897f3bc50c9 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -345,12 +346,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0012-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v7 11/16] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 49 +++++++----------------------
src/backend/access/heap/pruneheap.c | 22 ++++++++++---
src/include/access/heapam.h | 28 +++++++++--------
3 files changed, 44 insertions(+), 55 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index be48098f7f3..1c1785994b1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6340,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6657,8 +6657,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6711,30 +6711,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6746,20 +6733,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
}
MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
- false, /* no cleanup lock required */
- PRUNE_VACUUM_SCAN,
- tuples, ntuples,
- NULL, 0, /* redirected */
- NULL, 0, /* dead */
- NULL, 0); /* unused */
- }
-
- END_CRIT_SECTION();
}
/*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index fe463ad7146..8914d4bf5c8 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ log_heap_prune_and_freeze(relation, buffer,
+ frz_conflict_horizon, false, reason,
+ prstate.frozen, presult->nfrozen,
+ NULL, 0, /* redirected */
+ NULL, 0, /* dead */
+ NULL, 0); /* unused */
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index dbf6323b5ff..ac0ef6e4281 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -343,12 +344,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0012-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v7 11/16] Remove heap_freeze_execute_prepared()
@ 2024-03-26 13:10 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Melanie Plageman @ 2024-03-26 13:10 UTC (permalink / raw)
In order to merge freeze and prune records, the execution of tuple
freezing and the WAL logging of the changes to the page must be
separated so that the WAL logging can be combined with prune WAL
logging. This commit makes a helper for the tuple freezing and then
inlines the contents of heap_freeze_execute_prepared() where it is
called in heap_page_prune().
---
src/backend/access/heap/heapam.c | 49 +++++++----------------------
src/backend/access/heap/pruneheap.c | 22 ++++++++++---
src/include/access/heapam.h | 28 +++++++++--------
3 files changed, 44 insertions(+), 55 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index be48098f7f3..1c1785994b1 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6340,9 +6340,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
* XIDs or MultiXactIds that will need to be processed by a future VACUUM.
*
* VACUUM caller must assemble HeapTupleFreeze freeze plan entries for every
- * tuple that we returned true for, and call heap_freeze_execute_prepared to
- * execute freezing. Caller must initialize pagefrz fields for page as a
- * whole before first call here for each heap page.
+ * tuple that we returned true for, and then execute freezing. Caller must
+ * initialize pagefrz fields for page as a whole before first call here for
+ * each heap page.
*
* VACUUM caller decides on whether or not to freeze the page as a whole.
* We'll often prepare freeze plans for a page that caller just discards.
@@ -6657,8 +6657,8 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
-* Perform xmin/xmax XID status sanity checks before calling
-* heap_freeze_execute_prepared().
+* Perform xmin/xmax XID status sanity checks before actually executing freeze
+* plans.
*
* heap_prepare_freeze_tuple doesn't perform these checks directly because
* pg_xact lookups are relatively expensive. They shouldn't be repeated
@@ -6711,30 +6711,17 @@ heap_pre_freeze_checks(Buffer buffer,
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
+ * Helper which executes freezing of one or more heap tuples on a page on
+ * behalf of caller. Caller passes an array of tuple plans from
+ * heap_prepare_freeze_tuple. Caller must set 'offset' in each plan for us.
+ * Must be called in a critical section that also marks the buffer dirty and,
+ * if needed, emits WAL.
*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_freeze_prepared_tuples(Buffer buffer, HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- START_CRIT_SECTION();
-
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6746,20 +6733,6 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
}
MarkBufferDirty(buffer);
-
- /* Now WAL-log freezing if necessary */
- if (RelationNeedsWAL(rel))
- {
- log_heap_prune_and_freeze(rel, buffer, snapshotConflictHorizon,
- false, /* no cleanup lock required */
- PRUNE_VACUUM_SCAN,
- tuples, ntuples,
- NULL, 0, /* redirected */
- NULL, 0, /* dead */
- NULL, 0); /* unused */
- }
-
- END_CRIT_SECTION();
}
/*
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index fe463ad7146..8914d4bf5c8 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -635,10 +635,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(relation, buffer,
- frz_conflict_horizon,
- prstate.frozen, presult->nfrozen);
+ START_CRIT_SECTION();
+
+ Assert(presult->nfrozen > 0);
+
+ heap_freeze_prepared_tuples(buffer, prstate.frozen, presult->nfrozen);
+
+ MarkBufferDirty(buffer);
+
+ /* Now WAL-log freezing if necessary */
+ if (RelationNeedsWAL(relation))
+ log_heap_prune_and_freeze(relation, buffer,
+ frz_conflict_horizon, false, reason,
+ prstate.frozen, presult->nfrozen,
+ NULL, 0, /* redirected */
+ NULL, 0, /* dead */
+ NULL, 0); /* unused */
+
+ END_CRIT_SECTION();
}
else if (!pagefrz || !presult->all_frozen || presult->nfrozen > 0)
{
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index dbf6323b5ff..ac0ef6e4281 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -14,6 +14,7 @@
#ifndef HEAPAM_H
#define HEAPAM_H
+#include "access/heapam_xlog.h"
#include "access/relation.h" /* for backward compatibility */
#include "access/relscan.h"
#include "access/sdir.h"
@@ -101,8 +102,8 @@ typedef enum
} HTSV_Result;
/*
- * heap_prepare_freeze_tuple may request that heap_freeze_execute_prepared
- * check any tuple's to-be-frozen xmin and/or xmax status using pg_xact
+ * heap_prepare_freeze_tuple may request that any tuple's to-be-frozen xmin
+ * and/or xmax status is checked using pg_xact during freezing execution.
*/
#define HEAP_FREEZE_CHECK_XMIN_COMMITTED 0x01
#define HEAP_FREEZE_CHECK_XMAX_ABORTED 0x02
@@ -154,14 +155,14 @@ typedef struct HeapPageFreeze
/*
* "Freeze" NewRelfrozenXid/NewRelminMxid trackers.
*
- * Trackers used when heap_freeze_execute_prepared freezes, or when there
- * are zero freeze plans for a page. It is always valid for vacuumlazy.c
- * to freeze any page, by definition. This even includes pages that have
- * no tuples with storage to consider in the first place. That way the
- * 'totally_frozen' results from heap_prepare_freeze_tuple can always be
- * used in the same way, even when no freeze plans need to be executed to
- * "freeze the page". Only the "freeze" path needs to consider the need
- * to set pages all-frozen in the visibility map under this scheme.
+ * Trackers used when tuples will be frozen, or when there are zero freeze
+ * plans for a page. It is always valid for vacuumlazy.c to freeze any
+ * page, by definition. This even includes pages that have no tuples with
+ * storage to consider in the first place. That way the 'totally_frozen'
+ * results from heap_prepare_freeze_tuple can always be used in the same
+ * way, even when no freeze plans need to be executed to "freeze the
+ * page". Only the "freeze" path needs to consider the need to set pages
+ * all-frozen in the visibility map under this scheme.
*
* When we freeze a page, we generally freeze all XIDs < OldestXmin, only
* leaving behind XIDs that are ineligible for freezing, if any. And so
@@ -343,12 +344,13 @@ extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
extern void heap_pre_freeze_checks(Buffer buffer,
HeapTupleFreeze *tuples, int ntuples);
-extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples);
+
+extern void heap_freeze_prepared_tuples(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern bool heap_freeze_tuple(HeapTupleHeader tuple,
TransactionId relfrozenxid, TransactionId relminmxid,
TransactionId FreezeLimit, TransactionId MultiXactCutoff);
+
extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
const struct VacuumCutoffs *cutoffs,
TransactionId *NoFreezePageRelfrozenXid,
--
2.40.1
--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0012-Merge-prune-and-freeze-records.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-03-26 13:10 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-10-22 14:00 BUG #15449: file_fdw using program cause exit code error when using LIMIT PG Bug reporting form <[email protected]>
2018-10-22 17:27 ` Tom Lane <[email protected]>
2018-10-23 12:21 ` Tom Lane <[email protected]>
2018-10-29 06:58 ` Kyotaro HORIGUCHI <[email protected]>
2023-02-15 00:56 Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Kyotaro Horiguchi <[email protected]>
2023-02-15 08:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-02-16 21:21 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Andres Freund <[email protected]>
2023-03-06 07:33 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Drouvot, Bertrand <[email protected]>
2023-03-16 06:29 ` Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry Michael Paquier <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-01-07 22:03 [PATCH v4 12/19] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v9 11/21] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v7 11/16] Remove heap_freeze_execute_prepared() Melanie Plageman <[email protected]>
2024-03-26 13:10 [PATCH v7 11/16] Remove heap_freeze_execute_prepared() Melanie Plageman <[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