public inbox for [email protected]help / color / mirror / Atom feed
Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry 6+ messages / 5 participants [nested] [flat]
* 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2024-01-07 22:03 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 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 ` Drouvot, Bertrand <[email protected]> 2023-02-16 21:21 ` Andres Freund <[email protected]> 2023-03-06 07:33 ` Drouvot, Bertrand <[email protected]> 2023-03-16 06:29 ` Michael Paquier <[email protected]> 2024-01-07 22:03 [PATCH v4 12/19] 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