Re: [HACKERS] [PATCH] Call RelationDropStorage() for broader range ofobject drops.

On Wed, Nov 29, 2017 at 10:33 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Sep 15, 2017 at 4:36 AM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>> +1,
>> FDW looks OK for prototyping pluggable storage, but it doesn't seem suitable
>> for permanent implementation.
>> BTW, Hadi, could you visit "Pluggable storage" thread and check how suitable
>> upcoming pluggable storage API is for cstore?
>
> I cannot make a clear opinion about this patch. Not changing the
> situation, or changing it have both downsides and upsides. The
> suggestion from Alexander is surely something to look at. I am bumping
> this to next CF for now..

Well, I think there's no question that this patch is not really the
right way of solving the problem; the right way is pluggable storage.
What remains to be decided is whether we should adopt this approach to
make life easier for extension authors between now and then.  I don't
have a big problem with adopting this approach *provided that* we're
confident that it won't ever put us at risk of removing the wrong
files.  For example, if it were possible for rel->rd_node.relNode,
which we expect to be pointing at nothing in the case of a foreign
table, to instead be pointing at somebody else's relfilenode, this
patch would be bad news.

And I'm worried that might actually be a problem, because the only
hard guarantee GetNewRelFileNode() provides is that the file doesn't
exist on disk.  If the pg_class argument to that function is passed as
non-NULL, we additionally guarantee that the OID is not in use in
pg_class.oid, but not all callers do that, and it anyway doesn't
guarantee that the OID is not in use in pg_class.relfilenode.  So I
think it might be possible for a table that gets rewritten by CLUSTER
or VACUUM FULL to end up with the same relfilenode that was previously
assigned to a foreign table; dropping the foreign table would then
nuke the other relation's storage.  This probably wouldn't be a
problem in Citus's case, because they would create a file for the
FDW's relfilenode and that would prevent it from getting used -- but
it would be a problem for postgres_fdw or any other FDW which is doing
the expected thing of not creating files on disk.

Unless somebody can prove convincingly that this argument is wrong and
that there are no other possible problems either, and memorialize that
argument in the form of a detailed comment, I think we should reject
this patch.  http://postgr.es/m/CA+Tgmoa7TJA6-MvjWdcb_bouwFKx9apnU+Ok9m94TkTZTYmqjw@mail.gmail.com
from earlier this morning is good evidence for the proposition that we
must be careful to document the reasons for the changes we make, even
seemingly minor ones, if we don't want developers to be guessing in
ten years whether those changes were actually safe and correct.

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