Fix cloning with extensions in non default schema #717
Conversation
kbarber
changed the title
|
@dimitri I was trying to attack the problem from the |
kbarber
changed the title
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
ffa250b
to
b706b9c
Compare
|
Tested this on our instance, it works🎉 Trace |
There was a problem hiding this comment.
Well this is passing all our testing, which include filtering on namespaces. The extension test case also passes, where we deal with extensions separately from the pgcopydb clone command.
The trick here is to be compatible with a target database where the extensions have already been installed in a previous step, out of the pgcopydb scope. What happens with your patch if you create the extension (and its schema) manually in the target instance before running pgcopydb clone?
So if I use --drop-if-exists, it seems to work. If I do not, I get this error: |
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
79396d0
to
8588e7a
Compare
|
I think your test fails because of the code you deleted that handles pre-existing SQL objects in the target database... we need to keep that feature when adding your bug fix. |
Yeah without that ^^ the schemas don't get created for the extensions, and we hard fail. It looks like it wasn't filtering on what was in the target database, just the source database. Is that what needs to be fixed here perhaps? Do we need a t_namspace or something to populate the filter? So it sounds like the desire is: We need to be able to support copying extensions and having their schemas created if they don't exist, if the schema exists we shouldn't throw an error. Is this because we imagine some users to want to precreate their schemas before a clone? One idea might also to have a switch for There is another way this could be done. I mean create schema if not exists is a thing, but pg_dump I'm not sure it can drop that. We could pull out the pre-schema creation too I guess and make it more programmatic like the Although I am wondering about the consistency here: This seems contrary to what I'm seeing for tables however, which will always fail if you try to clone again without --drop-if-exists. For example: |
|
I added a |
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
cdc8f07
to
7bd6ad8
Compare
|
@dimitri do you think this is enough for now for just the |
src/bin/pgcopydb/catalog.c
Outdated
| /* | ||
| * Implement --skip-schemas | ||
| */ |
There was a problem hiding this comment.
What schemas are we skipping here? I think the comment needs more explaining.
There was a problem hiding this comment.
Added a better comment
docs/include/clone.rst
Outdated
| @@ -23,6 +23,7 @@ | |||
| --skip-ext-comments Skip restoring COMMENT ON EXTENSION | |||
| --skip-collations Skip restoring collations | |||
| --skip-vacuum Skip running VACUUM ANALYZE | |||
| --skip-schemas Skip creating schemas | |||
There was a problem hiding this comment.
Which schema are skipped with this option? I think the short text could be better, but also the long text covering the option in details is missing.
There was a problem hiding this comment.
Improved the text here, am I missing a doc update somewhere for a longer description?
There was a problem hiding this comment.
See https://pgcopydb.readthedocs.io/en/latest/ref/pgcopydb_clone.html#options
Also would be good if the options for skipping extension related stuff (comments, schemas) were grouped together in the listing.
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
2 times, most recently
from
cef67f6
to
3f8c7b0
Compare
|
@dimitri how about |
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
3f8c7b0
to
e1aa7ef
Compare
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
e1aa7ef
to
d344e9a
Compare
|
Greetings @dimitri would you be able to take a look at this PR again when you have time? Thanks. |
Yes, please have a look at the following part of the code: pgcopydb/src/bin/pgcopydb/dump_restore.c Lines 661 to 672 in 3f07e0b I would expect that we implement something similar for extensions schemas too, which means we populate the target database with the list of namespaces that extensions depend on. Do you think you could add that to your PR, basically making the |
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
3 times, most recently
from
e99808f
to
359682f
Compare
@dimitri thanks for responding. The code you pointed out and I think I fixed this in the current PR code feel free to take a look. I added a change to the test file to confirm with a creation of |
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
2 times, most recently
from
85bb3cd
to
c2a33fe
Compare
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
2 times, most recently
from
fb096f3
to
27d9979
Compare
There was a problem hiding this comment.
Nice! Thanks for the detective work. I like that you found that we match on schema name and owner and should only match on schema name. About the implementation, see my comments. Is there a way that we can lookup the actual schema name from the schema OID, if we have it, rather than parse the restore list name?
src/bin/pgcopydb/copydb_schema.c
Outdated
| copydb_schema_name_from_restore_list_name(const char *restoreListName, | ||
| char *schemaName) | ||
| { | ||
| if (!(sscanf(restoreListName, "- %64s %*s", schemaName) == 1)) /* IGNORE-BANNED */ |
There was a problem hiding this comment.
This is a very hopeful way to parse the restore_list_name. When a schema name includes double-quotes, these characters are escaped by being doubled, so we might have up to 128 characters in a schema name. The schema name might contain a space too. The owner name could contain a space also.
Does this archive Table of Contents line contain the schema OID? if yes, we should probably fill-in a SQLite catalog with the list of all the source schemas and then implement an OID lookup in that internal catalog. The way restore_list_name are built is not machine-parsable in the general case.
There was a problem hiding this comment.
Yeah, I was worrying about it too.
It completely fails to skips anything with a space, this is a toc sample I was looking at from pre-filtered.list.
;12; 2615 18530 SCHEMA - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabcdefghijklmnop fqayuutgtrkspv
;11; 2615 18529 SCHEMA - aabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghik fqayuutgtrkspv
13; 2615 18531 SCHEMA - abcdefghijklmnopqrstuv wxyz fqayuutgtrkspv
;10; 2615 18528 SCHEMA - abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghik fqayuutgtrkspv
There is the OID there, 18530, 18529, 18531.
So the answer you are describing is: do a lookup on the source catalog for the oids from s_namespace retrieving the nspame and then use that to check if the target has already got that nspname in s_namespace.
There was a problem hiding this comment.
The lookup should be done in the SQLite catalogs, and for that we need to create and fill-in a new s_namespace catalog table in the sourceDB. Once we have that we can lookup the SCHEMA entries by OID in the sourceDB to fetch the schema name (instead of parsing it from the archive contents), and then we can lookup the schema by name in the target database using what you did already.
So yes what you said, with the explicit caching of the whole pg_namespace catalog from the source database in our internal SQLite catalogs.
There was a problem hiding this comment.
Okay, thats done now I think. I added s_namepace to target, collected the schemas earlier and now we do a lookup by oid, get the name, then lookup that name. Seems to skip the schemas now with spaces in them:
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 12: SCHEMA 18530 - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabcdefghijklmnop fqayuutgtrkspv
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 11: SCHEMA 18529 - aabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghik fqayuutgtrkspv
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 13: SCHEMA 18531 - abcdefghijklmnopqrstuv wxyz fqayuutgtrkspv
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 10: SCHEMA 18528 - abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghik fqayuutgtrkspv
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 9: SCHEMA 18105 - foo fqayuutgtrkspv
10:54:06.125 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 7: SCHEMA 17937 - heroku_ext u2sc2mm8l1f6ht
10:54:06.126 620 NOTICE dump_restore.c:678 Skipping already existing dumpId 8: SCHEMA 18104 - public fqayuutgtrkspv
I'm just hitting a bug with the insert now into s_namespace throwing an error in some of our tests, I'll try and track that down.
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
27d9979
to
419ec29
Compare
src/bin/pgcopydb/schema.h
Outdated
| DATA_SECTION_ALL | ||
| DATA_SECTION_ALL, | ||
| DATA_SECTION_NAMESPACES | ||
| } CopyDataSection; | ||
|
|
||
| #define DATA_SECTION_COUNT (DATA_SECTION_ALL + 1) | ||
| #define DATA_SECTION_COUNT (DATA_SECTION_NAMESPACES + 1) |
There was a problem hiding this comment.
Please keep DATA_SECTION_ALL as the last entry in the list, and then DATA_SECTION_COUNT definition does not need to change either. I think I would insert DATA_SECTION_NAMESPACES just before DATA_SECTION_TABLE_DATA in the enum.
There was a problem hiding this comment.
Yep easy fix, thanks
@dimitri okay the code is now retrieving the namespaces for the source catalog, and registering the section as it expects so that commands can be repeated safely. It uses that information to lookup the nspname from source catalog using the oid from the archive list, then uses that name on the target catalog to check if it already exists or not. Looks like its passing, I modified the test to include schemas with spaces to help with regression. |
This removes the non-default schema exclusion code to allow extensions to be transfered with `clone` without interruption. We add tests to avoid regression.
kbarber
force-pushed
the
pgcopydb_clone_with_extensions_from_different_schemas
branch
from
76e8e1a
to
080ad11
Compare
@dimitri Yeah I was wondering about the ordering myself. Okay I've fixed that, rebased, squashed and the tests are passing again. |
Last round of review now, I think we're only missing registering the cache-invalidation for the namespace list at this place in the code, where I would add the DATA_SECTION_NAMESPACES just before DATA_SECTION_TABLE_DATA: pgcopydb/src/bin/pgcopydb/copydb_schema.c Lines 233 to 241 in 3f07e0b
Thanks for that, that's good thinking! |
|
Thanks for all the work @kbarber ; we end-up in a much different place than we started at, with a long way to get there, and the result is much better than what we started with. Now, I believe the other needed PR is going to be much simpler with just adding the SCHEMA in the CREATE EXTENSION command, I'm not sure anything else is needed really. |
|
Yeah thanks @dimitri it was a lot of fun learning how your tool works. Thanks for your patience with me. Yes, I imagine the other patch looks very different now, but for now, I'm taking a break and going shopping. Thanks again. |
kbarber
deleted the
pgcopydb_clone_with_extensions_from_different_schemas
branch
Related to: #711 and #694
So using this DDL on the sourcedb:
This patch attempts to fix the clone failure when there are extensions in different schemas due to the lack of the schema being created correctly beforehand.
For example: