On 12/06/2017 11:26 AM, Andrew Dunstan wrote: >> In general, you need to be thinking about this in terms of making sure >> that it's impossible to accidentally not update code that needs to be >> updated. Another example is that I noticed that some places the patch >> has s/CreateTupleDescCopy/CreateTupleDescCopyConstr/, evidently because >> the former will fail to copy the missing-attribute support data. >> I think that's an astonishingly bad idea. There is basically no situation >> in which CreateTupleDescCopy defined in that way will ever be safe to use. >> The missing-attribute info is now a fundamental part of a tupdesc and it >> has to be copied always, just as much as e.g. atttypid. >> >> I would opine that it's a mistake to design TupleDesc as though the >> missing-attribute data had anything to do with default values. It may >> have originated from the same place but it's a distinct thing. Better >> to store it in a separate sub-structure. > > OK, will work on all that. Thanks again. >
Looking closer at this. First, there is exactly one place where the patch does s/CreateTupleDescCopy/CreateTupleDescCopyConstr/. making this like atttypid suggests that it belongs right outside the TupleConstr structure. But then it seems to me that the change could well end up being a darn site more invasive. For example, should we be passing the number of missing values to CreateTemplateTupleDesc and CreateTupleDesc? I'm happy to do whatever work is required, but I want to have a firm understanding of the design before I spend lots of time cutting code. Once I understand how tupdesc.h should look I should be good. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
