24.01.2025 07:22, Shlok Kyal пишет:
On Thu, 23 Jan 2025 at 17:54, Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
On Thursday, January 23, 2025 4:43 PM Shlok Kyal <shlok.kyal....@gmail.com>
wrote:
On Thu, 23 Jan 2025 at 12:35, Shlok Kyal <shlok.kyal....@gmail.com> wrote:
On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
Thanks for the patch.
I agree that covering the partitioned table case when checking the
non-local origin data on publisher is an improvement. But I think
adding or extending the SQL functions may not be the appropriate way
to fix because the new functions cannot be used in older PG version and is
also not backpatchable.
I am thinking it would be better to use the existing
pg_partition_ancestors() and pg_partition_tree() to verify the same,
which can be used in all supported PG versions and is also backpatchable.
And here is another version which fixed the issue like that. I have
not added tests for it, but I think it's doable to write the
something like the testcases provided by Sergey. This patch does not
fix the foreign tabel as that seems to be a separate issue which can be fixed
independtly.
Hi Sergey, if you have the time, could you please verify whether
this patch resolves the partition issue you reported? I've confirmed
that it passes the partitioned tests in the scripts, but I would
appreciate your confirmation for the same.
Hi Hou-san,
I tested the patch, and it is working fine on HEAD.
I also tried to apply the patches to back branches PG17 and PG 16. But
the patch does not apply.
This 'origin' option was added in PG 16. So, this patch will not be
required for PG 15 and back branches.
I have created a patch which applies to both PG17 and PG 16. The
v6-0002 is the test patch. It applies to all the branches (HEAD, PG17,
PG16) correctly.
Thanks for the patch. I think the testcases could be improved.
It's not clear why a separate schema is created for tables. I assume it was
initially intended to test TABLES IN SCHEMA but was later modified. If the
separate schema is still necessary, could you please add comments to clarify
its purpose?
Besides, the new table name 'ts' seems a bit unconventional. It would be better
to align with the naming style of existing test cases for consistency and
clarity.
I think the schema is not required. I have removed it.
Also, Sergey had suggested adding an more test to verify scenarios where the
table's ancestors are subscribed. It appears this hasn't been added yet. I
think it would be better to add it.
I have added the test in the latest patch.
Thanks and Regards,
Shlok Kyal
Hello!
I think there is another problem - publisher can modify publication
during the execution of "CREATE SUBSCRIPTION" (Rarely, this situation
occurred in my tests.)
I.e.:
1. Publisher: CREATE PUBLICATION ... FOR TABLE t1;
2. Subscriber (starts CREATE SUBSCRIPTION ...): check_publications_origin()
3. Publisher: ALTER PUBLICATION ... ADD TABLE t2;
4. Subscriber (still process CREATE SUBSCRIPTION ...): fetch_table_list()
So, we check publication with only t1, but fetch t1,t2
I think we must start transaction on publisher while executing
CreateSubscription() on subscriber (Or may be take an lock on publisher )
Thoughts?
