postgres_fdw fails to see that array type belongs to extension

David Geier <geidav.pg@gmail.com> writes:

The filter clause of the query SELECT * FROM test WHERE col = 'foo' OR
col = 'bar' is pushed down to the remote, while the filter clause of the
semantically equivalent query SELECT * FROM test WHERE col IN ('foo',
'bar') is not.

I traced this down to getExtensionOfObject() called from
lookup_shippable(). getExtensionOfObject() doesn't recurse but only
checks first level dependencies and only checks for extension
dependencies. However, the IN operator takes an array of our custom data
type as argument (type is typically prefixed with _ in pg_type). This
array type is only dependent on our extension via the custom data type
in two steps which postgres_fdw doesn't see. Therefore, postgres_fdw
doesn't allow for push-down of the IN.

Hmm. It seems odd that if an extension defines a type, the type is
listed as a member of the extension but the array type is not.
That makes it look like the array type is an externally-created
thing that happens to depend on the extension, when it's actually
part of the extension. I'm surprised we've not run across other
misbehaviors traceable to that.

Of course, fixing it like that leads to needing to change the
contents of pg_depend, so it wouldn't be back-patchable. But it
seems like the best way in the long run.

regards, tom lane

Hi,

On 12/27/23 18:38, Tom Lane wrote:

Hmm. It seems odd that if an extension defines a type, the type is
listed as a member of the extension but the array type is not.
That makes it look like the array type is an externally-created
thing that happens to depend on the extension, when it's actually
part of the extension. I'm surprised we've not run across other
misbehaviors traceable to that.

Agreed.

Of course, fixing it like that leads to needing to change the
contents of pg_depend, so it wouldn't be back-patchable. But it
seems like the best way in the long run.

The attached patch just adds a 2nd dependency between the array type and
the extension, using recordDependencyOnCurrentExtension(). It seems like
that the other internal dependency on the element type must stay? If
that seems reasonable I can add a test to modules/test_extensions. Can
extensions in contrib use test extension in their own tests? It looks
like postgres_fdw doesn't test any of the shippability logic.

--
David Geier
(ServiceNow)

Hi,

I realized that ALTER EXTENSION foo ADD TYPE _bar does pretty much the
same via ExecAlterExtensionContentsStmt(). So the code in the patch
seems fine.

On 1/8/24 12:21, David Geier wrote:

The attached patch just adds a 2nd dependency between the array type
and the extension, using recordDependencyOnCurrentExtension(). It
seems like that the other internal dependency on the element type must
stay? If that seems reasonable I can add a test to
modules/test_extensions. Can extensions in contrib use test extension
in their own tests? It looks like postgres_fdw doesn't test any of the
shippability logic.

--
David Geier
(ServiceNow)

David Geier <geidav.pg@gmail.com> writes:

On 12/27/23 18:38, Tom Lane wrote:

Hmm. It seems odd that if an extension defines a type, the type is
listed as a member of the extension but the array type is not.
That makes it look like the array type is an externally-created
thing that happens to depend on the extension, when it's actually
part of the extension. I'm surprised we've not run across other
misbehaviors traceable to that.

Agreed.
The attached patch just adds a 2nd dependency between the array type and
the extension, using recordDependencyOnCurrentExtension().

I don't like this patch too much: it fixes the problem in a place far
away from GenerateTypeDependencies' existing treatment of extension
dependencies, and relatedly to that, fails to update the comments
it has falsified. I think we should do it more like the attached.
This approach means that relation rowtypes will also be explicitly
listed as extension members, which seems like it is fixing the same
sort of bug as the array case.

I also noted that you'd not run check-world, because there's a test
case that changes output. This is good though, because we don't need
to add any new test to prove it does what we want.

There's one big remaining to-do item, I think: experimentation with
pg_upgrade proves that a binary upgrade fails to fix the extension
membership of arrays/rowtypes. That's because pg_dump needs to
manually reconstruct the membership dependencies, and it thinks that
it doesn't need to do anything for implicit arrays. Normally the
point of that is that we want to exactly reconstruct the extension's
contents, but I think in this case we'd really like to add the
additional pg_depend entries even if they weren't there before.
Otherwise people wouldn't get the new behavior until they do a
full dump/reload.

I can see two ways we could do that:

* add logic to pg_dump

* modify ALTER EXTENSION ADD TYPE so that it automatically recurses
from a base type to its array type (and I guess we'd need to add
something for relation rowtypes and multiranges, too).

I think I like the latter approach because it's like how we
handle ownership: pg_dump doesn't emit any commands to explicitly
change the ownership of dependent types, either. (But see [1]/messages/by-id/1580383.1705343264@sss.pgh.pa.us.)
We could presumably steal some logic from ALTER TYPE OWNER.
I've not tried to code that here, though.

regards, tom lane

[1]: /messages/by-id/1580383.1705343264@sss.pgh.pa.us

I wrote:

There's one big remaining to-do item, I think: experimentation with
pg_upgrade proves that a binary upgrade fails to fix the extension
membership of arrays/rowtypes. That's because pg_dump needs to
manually reconstruct the membership dependencies, and it thinks that
it doesn't need to do anything for implicit arrays. Normally the
point of that is that we want to exactly reconstruct the extension's
contents, but I think in this case we'd really like to add the
additional pg_depend entries even if they weren't there before.
Otherwise people wouldn't get the new behavior until they do a
full dump/reload.

I can see two ways we could do that:

* add logic to pg_dump

* modify ALTER EXTENSION ADD TYPE so that it automatically recurses
from a base type to its array type (and I guess we'd need to add
something for relation rowtypes and multiranges, too).

I think I like the latter approach because it's like how we
handle ownership: pg_dump doesn't emit any commands to explicitly
change the ownership of dependent types, either. (But see [1].)
We could presumably steal some logic from ALTER TYPE OWNER.
I've not tried to code that here, though.

Now that the multirange issue is fixed (3e8235ba4), here's a
new version that includes the needed recursion in ALTER EXTENSION.
I spent some more effort on a proper test case, too.

regards, tom lane

I wrote:

Now that the multirange issue is fixed (3e8235ba4), here's a
new version that includes the needed recursion in ALTER EXTENSION.
I spent some more effort on a proper test case, too.

I looked this over again and pushed it.

regards, tom lane