Horton

Re: Declarative partitioning - another take

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Amit Langote <amitlangote09@gmail.com>, Dmitry Ivanov <d.ivanov@postgrespro.ru>, Michael Paquier <michael.paquier@gmail.com>, Andres Freund <andres@anarazel.de>, Erik Rijkers <er@xs4all.nl>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>, Pg Hackers <pgsql-hackers@postgresql.org>, pgsql-hackers-owner@postgresql.org
Date: 2017-01-24T17:56:36Z
Lists: pgsql-hackers 
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

Commits

  1. Fix typo.

  2. Document trigger-firing behavior for inheritance/partitioning.

  3. Fire per-statement triggers on partitioned tables.

  4. Set ecxt_scantuple correctly for tuple routing.

  5. Fix interaction of partitioned tables with BulkInsertState.

  6. Avoid core dump for empty prepared statement in an aborted transaction.

  7. Fix some problems in check_new_partition_bound().

  8. Remove unnecessary arguments from partitioning functions.

  9. Fix reporting of constraint violations for table partitioning.

  10. Fix tuple routing in cases where tuple descriptors don't match.

  11. Invalid parent's relcache after CREATE TABLE .. PARTITION OF.

  12. Doc: improve documentation about inheritance.