On Thu, Jan 19, 2017 at 9:58 PM, Amit Langote
<Langote_Amit_f8@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