Horton

Re: Declarative partitioning - another take

Amit Langote <langote_amit_f8@lab.ntt.co.jp>

View thread View on postgr.es
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: Robert Haas <robertmhaas@gmail.com>
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-25T01:33:28Z
Lists: pgsql-hackers

Attachments

On 2017/01/25 2:56, Robert Haas wrote:
> 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.

You're 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.

Agreed.  Updated patch attached.

Thanks,
Amit

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.