Обсуждение: postgres_fdw versus regconfig and similar constants

Bug #17483 points out that postgres_fdw falls down pretty badly when
a potentially shippable clause contains a "regconfig" constant [1].
It doesn't check whether the constant refers to an object that's
likely to exist on the remote side, and it fails to ensure that
the printed name is properly schema-qualified.  The same flaws apply
to constants of other OID alias types.  Below are some draft patches
to address this.

0001 deals with the lack-of-schema-qualification issue by forcing
search_path to be just "pg_catalog" while we're deparsing constants.
This seems straightforward, if annoyingly expensive, and it's enough
to fix the bug as presented.

0002 tightens deparse.c's rules to only consider an OID alias constant
as shippable if the object it refers to is shippable.  This seems
obvious in hindsight; I wonder how come we've not realized it before?
However, this has one rather nasty problem for regconfig in particular:
with our standard shippability rules none of the built-in text search
configurations would be treated as shippable, because initdb gives them
non-fixed OIDs above 9999.  That seems like a performance hit we don't
want to take.  In the attached, I hacked around that by making a special
exception for OIDs up to 16383, but that seems like a fairly ugly kluge.
Anybody have a better idea?

While using find_expr_references() as a reference for writing the new code
in 0002, I was dismayed to find that it omitted handling regcollation;
and a quick search showed that other places that specially process REG*
types hadn't been updated for that addition either.  0003 closes those
oversights.

I've split this into three parts partially because they probably should be
back-patched differently.  It seems like 0001 should go into all branches.
0003 should go back to v13 where regcollation was added.  But I wonder if
0002 should get back-patched at all: it seems like we're more likely to
get performance complaints about quals no longer being shipped than we are
to get kudos for not mistakenly shipping an unportable tsconfig reference.
People could fix such performance issues by putting the config into an
extension marked safe-to-ship, but they probably won't want to have to
deal with that in a minor release.

I've not done anything about a regression test yet.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/17483-795757fa99607659%40postgresql.org

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d56951153b..2ff6debebf 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3899,6 +3899,14 @@ set_transmission_modes(void)
                                  PGC_USERSET, PGC_S_SESSION,
                                  GUC_ACTION_SAVE, true, 0, false);

+    /*
+     * In addition force restrictive search_path, in case there are any
+     * regproc or similar constants to be printed.
+     */
+    (void) set_config_option("search_path", "pg_catalog",
+                             PGC_USERSET, PGC_S_SESSION,
+                             GUC_ACTION_SAVE, true, 0, false);
+
     return nestlevel;
 }

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 8f4d8a5022..a9766f9734 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -37,11 +37,14 @@
 #include "access/sysattr.h"
 #include "access/table.h"
 #include "catalog/pg_aggregate.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_namespace.h"
 #include "catalog/pg_operator.h"
 #include "catalog/pg_opfamily.h"
 #include "catalog/pg_proc.h"
+#include "catalog/pg_ts_config.h"
+#include "catalog/pg_ts_dict.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "nodes/makefuncs.h"
@@ -384,6 +387,75 @@ foreign_expr_walker(Node *node,
             {
                 Const       *c = (Const *) node;
 
+                /*
+                 * Constants of regproc and related types can't be shipped
+                 * unless the referenced object is shippable.  But NULL's ok.
+                 * (See also the related code in dependency.c.)
+                 */
+                if (!c->constisnull)
+                {
+                    switch (c->consttype)
+                    {
+                        case REGPROCOID:
+                        case REGPROCEDUREOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              ProcedureRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGOPEROID:
+                        case REGOPERATOROID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              OperatorRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGCLASSOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              RelationRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGTYPEOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              TypeRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGCOLLATIONOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              CollationRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGCONFIGOID:
+
+                            /*
+                             * For text search objects only, we weaken the
+                             * normal shippability criterion to allow all OIDs
+                             * below FirstNormalObjectId.  Without this, none
+                             * of the initdb-installed TS configurations would
+                             * be shippable, which would be quite annoying.
+                             */
+                            if (DatumGetObjectId(c->constvalue) >= FirstNormalObjectId &&
+                                !is_shippable(DatumGetObjectId(c->constvalue),
+                                              TSConfigRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGDICTIONARYOID:
+                            if (DatumGetObjectId(c->constvalue) >= FirstNormalObjectId &&
+                                !is_shippable(DatumGetObjectId(c->constvalue),
+                                              TSDictionaryRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGNAMESPACEOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              NamespaceRelationId, fpinfo))
+                                return false;
+                            break;
+                        case REGROLEOID:
+                            if (!is_shippable(DatumGetObjectId(c->constvalue),
+                                              AuthIdRelationId, fpinfo))
+                                return false;
+                            break;
+                    }
+                }
+
                 /*
                  * If the constant has nondefault collation, either it's of a
                  * non-builtin type, or it reflects folding of a CollateExpr.
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index de10923391..f10abd78b7 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1840,6 +1840,13 @@ find_expr_references_walker(Node *node,
                         add_object_address(OCLASS_TYPE, objoid, 0,
                                            context->addrs);
                     break;
+                case REGCOLLATIONOID:
+                    objoid = DatumGetObjectId(con->constvalue);
+                    if (SearchSysCacheExists1(COLLOID,
+                                              ObjectIdGetDatum(objoid)))
+                        add_object_address(OCLASS_COLLATION, objoid, 0,
+                                           context->addrs);
+                    break;
                 case REGCONFIGOID:
                     objoid = DatumGetObjectId(con->constvalue);
                     if (SearchSysCacheExists1(TSCONFIGOID,
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index fa1f589fad..1884918318 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -4326,6 +4326,7 @@ convert_to_scalar(Datum value, Oid valuetypid, Oid collid, double *scaledvalue,
         case REGOPERATOROID:
         case REGCLASSOID:
         case REGTYPEOID:
+        case REGCOLLATIONOID:
         case REGCONFIGOID:
         case REGDICTIONARYOID:
         case REGROLEOID:
@@ -4457,6 +4458,7 @@ convert_numeric_to_scalar(Datum value, Oid typid, bool *failure)
         case REGOPERATOROID:
         case REGCLASSOID:
         case REGTYPEOID:
+        case REGCOLLATIONOID:
         case REGCONFIGOID:
         case REGDICTIONARYOID:
         case REGROLEOID:
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 6ae7c1f50b..38e943fab2 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -240,6 +240,7 @@ GetCCHashEqFuncs(Oid keytype, CCHashFN *hashfunc, RegProcedure *eqfunc, CCFastEq
         case REGOPERATOROID:
         case REGCLASSOID:
         case REGTYPEOID:
+        case REGCOLLATIONOID:
         case REGCONFIGOID:
         case REGDICTIONARYOID:
         case REGROLEOID:

On Mon, May 16, 2022 at 1:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 0001 deals with the lack-of-schema-qualification issue by forcing
> search_path to be just "pg_catalog" while we're deparsing constants.
> This seems straightforward, if annoyingly expensive, and it's enough
> to fix the bug as presented.

Yeah, that does seem like the thing to do. I doubt it will be the last
problem setting we need to add to that list, either. It's kind of
unfortunate that data type output formatting is context-dependent like
this, but I don't have an idea.

> 0002 tightens deparse.c's rules to only consider an OID alias constant
> as shippable if the object it refers to is shippable.  This seems
> obvious in hindsight; I wonder how come we've not realized it before?
> However, this has one rather nasty problem for regconfig in particular:
> with our standard shippability rules none of the built-in text search
> configurations would be treated as shippable, because initdb gives them
> non-fixed OIDs above 9999.  That seems like a performance hit we don't
> want to take.  In the attached, I hacked around that by making a special
> exception for OIDs up to 16383, but that seems like a fairly ugly kluge.
> Anybody have a better idea?

No. It feels to me like there are not likely to be any really
satisfying answers here. We have a way of mapping a given local table
to a given foreign table, but to the best of my knowledge we have no
similar mechanism for any other type of object. So it's just crude
guesswork. Who is to say whether the fact that we have a local text
search configuration means that there is a remote text search
configuration with the same name, and even if yes, that it has the
same semantics? And similarly for any other object types? Every
release adds and occasionally removes SQL objects from the system
catalogs, and depending on the object type, it can also vary by
operating system. There are several multiple forks of PostgreSQL, too.

> While using find_expr_references() as a reference for writing the new code
> in 0002, I was dismayed to find that it omitted handling regcollation;
> and a quick search showed that other places that specially process REG*
> types hadn't been updated for that addition either.  0003 closes those
> oversights.

Makes sense.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 16, 2022 at 1:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 0002 tightens deparse.c's rules to only consider an OID alias constant
>> as shippable if the object it refers to is shippable.  This seems
>> obvious in hindsight; I wonder how come we've not realized it before?
>> However, this has one rather nasty problem for regconfig in particular:
>> with our standard shippability rules none of the built-in text search
>> configurations would be treated as shippable, because initdb gives them
>> non-fixed OIDs above 9999.  That seems like a performance hit we don't
>> want to take.  In the attached, I hacked around that by making a special
>> exception for OIDs up to 16383, but that seems like a fairly ugly kluge.
>> Anybody have a better idea?

> No. It feels to me like there are not likely to be any really
> satisfying answers here.

Yeah.  Hearing no better ideas from anyone else either, pushed that way.

I noted one interesting factoid while trying to make a test case for the
missing-schema-qualification issue.  I thought of making a foreign table
that maps to pg_class and checking what is shipped for

select oid, relname from remote_pg_class where oid =
'information_schema.key_column_usage'::regclass;

(In hindsight, this wouldn't have worked anyway after patch 0002,
because that OID would have been above 9999.)  But what I got was

 Foreign Scan on public.remote_pg_class  (cost=100.00..121.21 rows=4 width=68)
   Output: oid, relname
   Remote SQL: SELECT oid, relname FROM pg_catalog.pg_class WHERE ((oid = 13527::oid))

The reason for that is that the constant is smashed to type OID so hard
that we can no longer tell that it ever was regclass, thus there's no
hope of deparsing it in a more-symbolic fashion.  I'm not sure if there's
anything we could do about that that wouldn't break more things than
it fixes (e.g. by making things that should look equal() not be so).
But anyway, this effect may help explain the lack of previous complaints
in this area.  regconfig arguments to text search functions might be
pretty nearly the only realistic use-case for shipping symbolic reg*
values to the remote.

            regards, tom lane