On Tue, 13 Jun 2023 at 11:49, Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Fri, May 26, 2023 at 6:55 PM shveta malik <shveta.ma...@gmail.com> wrote: > > > > I have attempted to convert pg_get_indexdef() to use > > systable_beginscan() based on transaction-snapshot rather than using > > SearchSysCache(). The latter does not have any old info and thus > > provides only the latest info as per the committed txns, which could > > result in errors in some scenarios. One such case is mentioned atop > > pg_dump.c. The patch is an attempt to fix the same pg_dump's issue. > > Any feedback is welcome. > > Even only in pg_get_indexdef_worker(), there are still many places > where we use syscache lookup: generate_relation_name(), > get_relation_name(), get_attname(), get_atttypetypmodcoll(), > get_opclass_name() etc. > > > > > There is a long list of pg_get_* functions which use SearchSysCache() > > and thus may expose similar issues. I can give it a try to review the > > possibility of converting all of them. Thoughts? > > > > it would be going to be a large refactoring and potentially make the > future implementations such as pg_get_tabledef() etc hard. Have you > considered changes to the SearchSysCache() family so that they > internally use a transaction snapshot that is registered in advance. > Since we are already doing similar things for catalog lookup in > logical decoding, it might be feasible. That way, the changes to > pg_get_XXXdef() functions would be much easier.
I feel registering an active snapshot before accessing the system catalog as suggested by Sawada-san will be a better fix to solve the problem. If this approach is fine, we will have to similarly fix the other pg_get_*** functions like pg_get_constraintdef, pg_get_function_arguments, pg_get_function_result, pg_get_function_identity_arguments, pg_get_function_sqlbody, pg_get_expr, pg_get_partkeydef, pg_get_statisticsobjdef and pg_get_triggerdef. The Attached patch has the changes for the same. Regards, Vignesh
From c399e185725319e501482e44574eab5830bad0b3 Mon Sep 17 00:00:00 2001 From: Vignesh C <vignes...@gmail.com> Date: Wed, 14 Jun 2023 11:35:27 +0530 Subject: [PATCH] pg_get_indexdef modification to access catalog based on the TxnSnapshot. Change pg_get_indexdef() to access catalog based on transaction-snapshot by setting historic snapshot which will prevent accessing a future catalog data which might occur due to concurrent catalog change. The motivation is to fix the issue mentioned atop pg_dump.c --- src/backend/utils/adt/ruleutils.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index d3a973d86b..3f80c77a00 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -1154,10 +1154,17 @@ pg_get_indexdef(PG_FUNCTION_ARGS) prettyFlags = PRETTYFLAG_INDENT; + /* + * Setup the snapshot to get the catalog information using this snapshot + * which will prevent accessing a future catalog data which has occurred + * due to concurrent catalog change. + */ + SetupHistoricSnapshot(GetActiveSnapshot(), NULL); res = pg_get_indexdef_worker(indexrelid, 0, NULL, false, false, false, false, prettyFlags, true); + TeardownHistoricSnapshot(false); if (res == NULL) PG_RETURN_NULL(); -- 2.34.1
