Re: Bug in pg_dump

On Sun, Mar 1, 2015 at 1:09 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Michael,
>
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> + /*
>> + * Query all the foreign key dependencies for all the extension
>> + * tables found previously. Only tables whose data need to be
>> + * have to be tracked.
>> + */
>> + appendPQExpBuffer(query2,
>> + "SELECT conrelid, confrelid "
>> + "FROM pg_catalog.pg_constraint "
>> + "WHERE contype = 'f' AND conrelid IN (");
>> +
>> + for (j = 0; j < nconfigitems; j++)
>> + {
>
> [...]
>
> Instead of building up a (potentially) big list like this, couldn't we
> simply join against pg_extension and check if conrelid = ANY (extconfig)
> instead, based on the extension we're currently processing?
>
> eg:
>
> appendPQExpBuffer(query2,
> "SELECT conrelid, confrelid "
> "FROM pg_catalog.pg_constraint, pg_extension "
> "WHERE contype = 'f' AND extname = '%s' AND conrelid = ANY (extconfig)",
> fmtId(curext->dobj.name));
>
> This seemed to work just fine for me, at least, and reduces the size of
> the patch a bit further since we don't need the loop that builds up the
> ids.

Actually, I did a mistake in my last patch, see this comment:
+ * Now that all the TableInfoData objects have been created for
+ * all the extensions, check their FK dependencies and register
+ * them to ensure correct data ordering.

The thing is that we must absolutely wait for *all* the TableInfoData
of all the extensions to be created because we need to mark the
dependencies between them, and even my last patch, or even with what
you are proposing we may miss tracking of FK dependencies between
tables in different extensions. This has little chance to happen in
practice, but I think that we should definitely get things right.
Hence something like this query able to query all the FK dependencies
with tables in extensions is more useful, and it has no IN clause:
+ appendPQExpBuffer(query,
+ "SELECT conrelid, confrelid "
+ "FROM pg_constraint "
+ "LEFT JOIN pg_depend ON (objid = confrelid) "
+ "WHERE contype = 'f' "
+ "AND refclassid = 'pg_extension'::regclass "
+ "AND classid = 'pg_class'::regclass;");

>> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well
>
> This is really an independent thing, no? I don't see any particular
> problem with it, for my part.

Yes, that's an independent thing, but my guess is that it would be
good to have a real test case this time to be sure that this does not
break again, at least on master where test/modules is an ideal place.

Attached are updated patches, the fix of pg_dump can be easily
cherry-picked down to 9.2. For 9.1 things are changed a bit because of
the way SQL queries are run, still patches are attached for all the
versions needed. I tested as well the fix down to this version 9.1
using the test case dump_test.
Thanks,
--
Michael