On Wed, Nov 29, 2017 at 10:33 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Fri, Sep 15, 2017 at 4:36 AM, Alexander Korotkov > <a.korot...@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+ok9m94tktztym...@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
