On Thu, Jan 19, 2017 at 9:58 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: >> But I wonder why we don't instead just change this function to >> consider tdhasoid rather than tdtypeid. I mean, if the only point of >> comparing the type OIDs is to find out whether the table-has-OIDs >> setting matches, we could instead test that directly and avoid needing >> to pass an extra argument. I wonder if there's some other reason this >> code is there which is not documented in the comment... > > With the following patch, regression tests run fine: > > if (indesc->natts == outdesc->natts && > - indesc->tdtypeid == outdesc->tdtypeid) > + indesc->tdhasoid != outdesc->tdhasoid) > { > > If checking tdtypeid (instead of tdhasoid directly) has some other > consideration, I'd would have seen at least some tests broken by this > change. So, if we are to go with this, I too prefer it over my previous > proposal to add an argument to convert_tuples_by_name(). Attached 0003 > implements the above approach.
I think this is not quite right. First, the patch compares the tdhasoid status with != rather than ==, which would have the effect of saying that we can skip conversion of the has-OID statuses do NOT match. That can't be right. Second, I believe that the comments imply that conversion should be done if *either* tuple has OIDs. I believe that's because whoever wrote this comment thought that we needed to replace the OID if the tuple already had one, which is what do_convert_tuple would do. I'm not sure whether that's really necessary, but we're less likely to break anything if we preserve the existing behavior, and I don't think we lose much from doing so because few user tables will have OIDs. So I would change this test to if (indesc->natts == outdesc->natts && !indesc->tdhasoid && !outdesc->tdhasoid), and I'd revise the one in convert_tuples_by_position() to match. Then I think it's much clearer that we're just optimizing what's there already, not changing the behavior. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers