Обсуждение: pg_get_indexdef() modification to use TxnSnapshot
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. 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? thanks Shveta
Вложения
On Fri, 26 May 2023 at 15:25, shveta malik <shveta.malik@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. > > 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? I could reproduce this issue in HEAD with pg_dump dumping a database having a table and an index like: create table t1(c1 int); create index idx1 on t1(c1); Steps to reproduce: a) ./pg_dump -d postgres -f dump.txt -- Debug this statement and hold a breakpoint at getTables function just before it takes lock on the table t1 b) when the breakpoint is hit, drop the index idx1 c) Continue the pg_dump execution after dropping the index d) pg_dump calls pg_get_indexdef to get the index definition e) since pg_get_indexdef->pg_get_indexdef uses SearchSysCache1 which uses the latest transaction, it will not get the index information as it sees the latest catalog snapshot(it will not get the index information). e) pg_dump will get an empty index statement in this case like: --------------------------------------------------------------------------------------------- -- -- Name: idx; Type: INDEX; Schema: public; Owner: vignesh -- ; --------------------------------------------------------------------------------------------- This issue is solved using shveta's patch as it registers the transaction snapshot and calls systable_beginscan which will not see the transactions that were started after pg_dump's transaction(the drop index will not be seen) and gets the index definition as expected like: --------------------------------------------------------------------------------------------- -- -- Name: idx; Type: INDEX; Schema: public; Owner: vignesh -- CREATE INDEX idx ON public.t1 USING btree (c1); --------------------------------------------------------------------------------------------- Regards, Vignesh
On Fri, May 26, 2023 at 6:55 PM shveta malik <shveta.malik@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. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Tue, 13 Jun 2023 at 11:49, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Fri, May 26, 2023 at 6:55 PM shveta malik <shveta.malik@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
Вложения
Hi hackers,
With '0001-pg_get_indexdef-modification-to-access-catalog-based.patch'
patch,
I confirmed that definition information
can be collected even if the index is droped during pg_dump.
The regression test (make check-world) has passed.
I also tested the view definition for a similar problem.
As per the attached patch and test case,
By using SetupHistoricSnapshot for pg_get_viewdef() as well,
Similarly, definition information can be collected
for VIEW definitions even if the view droped during pg_dump.
The regression test (make check-world) has passed,
and pg_dump's SERIALIZABLE results are improved.
However, in a SERIALIZABLE transaction,
If you actually try to access it, it will cause ERROR,
so it seems to cause confusion.
I think the scope of this improvement should be limited
to the functions listed as pg_get_*** functions at the moment.
---
# test2 pg_get_indexdef,pg_get_viewdef
## create table,index,view
drop table test1;
create table test1(id int);
create index idx1_test1 on test1(id);
create view view1_test1 as select * from test1;
## begin Transaction-A
begin transaction isolation level serializable;
select pg_current_xact_id();
## begin Transaction-B
begin transaction isolation level serializable;
select pg_current_xact_id();
## drop index,view in Transaction-A
drop index idx1_test1;
drop view view1_test1;
commit;
## SELECT pg_get_indexdef,pg_get_viewdef in Transaction-B
SELECT pg_get_indexdef(oid) FROM pg_class WHERE relname = 'idx1_test1';
SELECT pg_get_viewdef(oid) FROM pg_class WHERE relname = 'view1_test1';
## correct info from index and view
postgres=*# SELECT pg_get_indexdef(oid) FROM pg_class WHERE relname =
'idx1_test1';
pg_get_indexdef
----------------------------------------------------------
CREATE INDEX idx1_test1 ON public.test1 USING btree (id)
(1 row)
postgres=*# SELECT pg_get_viewdef(oid) FROM pg_class WHERE relname =
'view1_test1';
pg_get_viewdef
----------------
SELECT id +
FROM test1;
(1 row)
## However, SELECT * FROM view1_test1 cause ERROR because view does not
exist
postgres=*# SELECT * FROM view1_test1;
ERROR: relation "view1_test1" does not exist
LINE 1: SELECT * FROM view1_test1;
Best Regards,
Keisuke Kuroda
NTT COMWARE
On 2023-06-14 15:31, vignesh C wrote:
> On Tue, 13 Jun 2023 at 11:49, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
>>
>> On Fri, May 26, 2023 at 6:55 PM shveta malik <shveta.malik@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
Вложения
kuroda.keisuke@nttcom.co.jp writes:
> On 2023-06-14 15:31, vignesh C wrote:
>> I have attempted to convert pg_get_indexdef() to use
>> systable_beginscan() based on transaction-snapshot rather than using
>> SearchSysCache().
Has anybody thought about the fact that ALTER TABLE ALTER TYPE
(specifically RememberIndexForRebuilding) absolutely requires seeing
the latest version of the index's definition?
>>> 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.
A very significant fraction of other SearchSysCache callers likewise
cannot afford to see stale data. We might be able to fix things so
that the SQL-accessible ruleutils functionality works differently, but
we can't just up and change the behavior of cache lookups everywhere.
regards, tom lane
On Thu, Oct 5, 2023 at 11:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > kuroda.keisuke@nttcom.co.jp writes: > > On 2023-06-14 15:31, vignesh C wrote: > >> I have attempted to convert pg_get_indexdef() to use > >> systable_beginscan() based on transaction-snapshot rather than using > >> SearchSysCache(). > > Has anybody thought about the fact that ALTER TABLE ALTER TYPE > (specifically RememberIndexForRebuilding) absolutely requires seeing > the latest version of the index's definition? > > >>> 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. > > A very significant fraction of other SearchSysCache callers likewise > cannot afford to see stale data. We might be able to fix things so > that the SQL-accessible ruleutils functionality works differently, but > we can't just up and change the behavior of cache lookups everywhere. This patch was registered in the CommitFest as a bug fix, but I think it's a much more significant change than that label applies, and it seems like we have no consensus on what the right design is. Since there's been no response to these (entirely valid) comments from Tom in the past 3 months, I've marked this CF entry RwF for now. -- Robert Haas EDB: http://www.enterprisedb.com