Re: BUG #16583: merge join on tables with different DB collation behind postgres_fdw fails

Tom Lane <tgl@sss.pgh.pa.us>

From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Cc: jurafejfar@gmail.com, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Date: 2020-08-18T20:09:44Z
Lists: pgsql-bugs, pgsql-hackers

Attachments

I wrote:
>> So I think what is happening here is that postgres_fdw's version of
>> IMPORT FOREIGN SCHEMA translates "COLLATE default" on the remote
>> server to "COLLATE default" on the local one, which of course is
>> a big fail if the defaults don't match.  That allows the local
>> planner to believe that remote ORDER BYs on the two foreign tables
>> will give compatible results, causing the merge join to not work
>> very well at all.

Here's a full patch addressing this issue.  I decided that the best
way to address the test-instability problem is to explicitly give
collations to all the foreign-table columns for which it matters
in the postgres_fdw test.  (For portability's sake, that has to be
"C" or "POSIX"; I mostly used "C".)  Aside from ensuring that the
test still passes with some other prevailing locale, this seems like
a good idea since we'll then be testing the case we are encouraging
users to use.

And indeed, it immediately turned up a new problem: if we explicitly
assign a collation to a foreign-table column c, the system won't
ship WHERE clauses as simple as "c = 'foo'" to the remote.  This
surprised me, but the reason turned out to be that what postgres_fdw
is actually seeing is something like

   {OPEXPR 
   :opno 98 
   :opfuncid 67 
   :opresulttype 16 
   :opretset false 
   :opcollid 0 
   :inputcollid 950 
   :args (
      {VAR 
      :varno 6 
      :varattno 4 
      :vartype 25 
      :vartypmod -1 
      :varcollid 950 
      :varlevelsup 0 
      :varnosyn 6 
      :varattnosyn 4 
      :location 171
      }
      {RELABELTYPE 
      :arg 
         {CONST 
         :consttype 25 
         :consttypmod -1 
         :constcollid 100 
         :constlen -1 
         :constbyval false 
         :constisnull false 
         :location 341 
         :constvalue 9 [ 36 0 0 0 48 48 48 48 49 ]
         }
      :resulttype 25 
      :resulttypmod -1 
      :resultcollid 950 
      :relabelformat 2 
      :location -1
      }
   )
   :location -1
   }

that is, the constant is being explicitly relabeled with the correct
collation, and thus is_foreign_expr() thinks the collation shown by
the RelabelType node is an unsafely-introduced collation.

What I did about this was to change the recursion rule in
foreign_expr_walker() so that merging a safely-derived collation with
the same collation unsafely derived is considered safe.  I think this
is all right, and it allows us to accept some cases that previously
were rejected as unsafe.  But I might be missing something.

(BTW, there's an independent bug here, which is that we're getting
a tree of the above shape rather than a simple Const with the
appropriate collation; that is, this tree isn't fully const-folded.
This is a bug in canonicalize_ec_expression, which I'll go fix
separately.  But it won't affect the problem at hand.)

This seems like a sufficiently large change in postgres_fdw's
behavior to require review, so I'll go add this to the next CF.

			regards, tom lane

Commits

  1. Doc: extend warnings about collation-mismatch hazards in postgres_fdw.

  2. Add support for asynchronous execution.

  3. Suppress unnecessary RelabelType nodes in yet more cases.