Re: [HACKERS] MERGE SQL Statement for PG11

On Sat, Mar 24, 2018 at 8:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 22, 2018 at 7:13 PM, Peter Geoghegan <pg@bowt.ie> wrote:
>> While I think this this particular HINT buglet is pretty harmless, I
>> continue to be concerned about the unintended consequences of having
>> multiple RTEs for MERGE's target table. Each RTE comes from a
>> different lookup path -- the first one goes through setTargetTable()'s
>> parserOpenTable() + addRangeTableEntryForRelation() calls. The second
>> one goes through transformFromClauseItem(), for the join, which
>> ultimately ends up calling transformTableEntry()/addRangeTableEntry().
>> Consider commit 5f173040, which fixed a privilege escalation security
>> bug around multiple name lookup. Could the approach taken by MERGE
>> here introduce a similar security issue?
>
> Yeah, that seems really bad.  I don't think there is a huge problem
> with having multiple RTEs; for example, we very commonly end up with
> both rte->inh and !rte->inh RTEs for the same table, and that is OK.
> However, generating those RTEs by doing multiple name lookups for the
> same table is a big problem.  Imagine, for example, that a user has a
> search_path of a, b and that there is a table b.foo.  The user does a
> merge on foo.  Between the first name lookup and the second, somebody
> creates a.foo.  Now, potentially, half of the MERGE statement is going
> to be running against b.foo and the other half against a.foo.  I don't
> know whether that will crash or bomb out with a strange error or just
> make some unexpected modification to one of those tables, but the
> behavior, even if not insecure, will certainly be wrong.

If it's possible to identify the two OIDs that are supposed to match
and cross-check that the OIDs are the same, then we could just bomb
out with an error if they aren't.  That's not lovely, and is basically
a hack, but it's possible that no better fix is possible in the time
we have, and it's wouldn't be any worse than this crock from copy.c:

            if (!list_member_oid(plan->relationOids, queryRelId))
                ereport(ERROR,
                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                         errmsg("relation referenced by COPY statement
has changed")));

Uggh, that code makes me hold my nose every time I look at it.  But
it's a cheap fix.  (Hmm... I wonder if that's really an adequate fix
for the problem in COPY, if we can't verify that the OID in question
plays the right role in the query, rather than just that it's there
somewhere.  Anyway, if we can reliably identify the two OIDs to be
compared, that's certainly good enough.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company