Re: Adding support for Default partition in partitioning

Hi Robert,

Please find my feedback inlined.
I have addressed following comments in V25 patch[1].

> > 0002:
> > This patch teaches the partitioning code to handle the NIL returned by
> > get_qual_for_list().
> > This is needed because a default partition will not have any constraints
> in
> > case
> > it is the only partition of its parent.
>
> Perhaps it would be better to make validatePartConstraint() a no-op
> when the constraint is empty rather than putting the logic in the
> caller. Otherwise, every place that calls validatePartConstraint()
> has to think about whether or not the constraint-is-NULL case needs to
> be handled.
>
> I have now added a check in ValidatePartConstraint(). This change is made
in 0001 patch.

> > 0003:
> > Support for default partition with the restriction of preventing
> addition of
> > any
> > new partition after default partition.
>
> This looks generally reasonable, but can't really be committed without
> the later patches, because it might break pg_dump, which won't know
> that the DEFAULT partition must be dumped last and might therefore get
> the dump ordering wrong, and of course also because it reverts commit
> c1e0e7e1d790bf18c913e6a452dea811e858b554.
>
>
Thanks Robert for looking into this. The purpose of having different
patches is
just to ease the review process and the basic patch of introducing the
default
partition support and extending support for addition of new partition
should go
together.

> > 0004:
> > Store the default partition OID in pg_partition_table, this will help us
> to
> > retrieve the OID of default relation when we don't have the relation
> cache
> > available. This was also suggested by Amit Langote here[1].
>
> I looked this over and I think this is the right approach. An
> alternative way to avoid needing a relcache entry in
> heap_drop_with_catalog() would be for get_default_partition_oid() to
> call find_inheritance_children() here and then use a syscache lookup
> to get the partition bound for each one, but that's still going to
> cause some syscache bloat.
>

To safeguard future development from missing this and leaving it out of
sync, I
have added an Assert in RelationBuildPartitionDesc() to cross check the
partdefid.

>
> > 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> + if (spec->is_default)
> + {
> + /* Default partition cannot be added if there already
> exists one. */
> + if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> + {
> + with = boundinfo->default_index;
> + ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> + relname,
> get_rel_name(partdesc->oids[with])),
> + parser_errposition(pstate,
> spec->location)));
> + }
> +
> + return;
> + }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level. In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented. Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.
>

Fixed.

> 0006:
> > Extend default partitioning validation code to reuse the refactored code
> in
> > patch 0001.
>
> I'm having a very hard time understanding what's going on with this
> patch. It certainly doesn't seem to be just refactoring things to use
> the code from 0001. For example:
>
> - if (ExecCheck(partqualstate, econtext))
> + if (!ExecCheck(partqualstate, econtext))
>
> It seems hard to believe that refactoring things to use the code from
> 0001 would involve inverting the value of this test.
>
> + * derived from the bounds(the partition constraint
> never evaluates to
> + * NULL, so negating it like this is safe).
>
> I don't see it being negated.
>
> I think this patch needs a better explanation of what it's trying to
> do, and better comments. I gather that at least part of the point
> here is to skip validation scans on default partitions if the default
> partition has been constrained not to contain any values that would
> fall in the new partition, but neither the commit message for 0006 nor
> your description here make that very clear.
>

In V25 series, this is now part of patch 0004, and should avoid any
confusion as above. I have tried to add verbose comment in commit
message as well as I have improved the code comments in this code
area.

> [0008 documentation]
>
> - attached is marked <literal>NO INHERIT</literal>, the command will
> fail;
> - such a constraint must be recreated without the <literal>NO
> INHERIT</literal>
> - clause.
> + target table.
> + </para>
>
> I don't favor inserting a paragraph break here.
>
> Fixed.

> + then the default partition(if it is a regular table) is scanned to
> check
>
> The sort-of-trivial problem with this is that an open parenthesis
> should be proceeded by a space. But I think this won't be clear. I
> think you should move this below the following paragraph, which
> describes what happens for foreign tables, and then add a new
> paragraph like this:
>
> When a table has a default partition, defining a new partition changes
> the partition constraint for the default partition. The default
> partition can't contain any rows that would need to be moved to the
> new partition, and will be scanned to verify that none are present.
> This scan, like the scan of the new partition, can be avoided if an
> appropriate <literal>CHECK</literal> constraint is present. Also like
> the scan of the new partition, it is always skipped when the default
> partition is a foreign table.
>
> I have made the change as suggested.

> -) ] FOR VALUES <replaceable
> class="PARAMETER">partition_bound_spec</replaceable>
> +) ] { DEFAULT | FOR VALUES <replaceable
> class="PARAMETER">partition_bound_spec</replaceable> }
>
> I recommend writing FOR VALUES | DEFAULT both here and in the ATTACH
> PARTITION syntax summary.
>
> Changed.

> + If <literal>DEFAULT</literal> is specified the table will be created
> as a
> + default partition of the parent table. The parent can either be a
> list or
> + range partitioned table. A partition key value not fitting into any
> other
> + partition of the given parent will be routed to the default
> partition.
> + There can be only one default partition for a given parent table.
> + </para>
> +
> + <para>
> + If the given parent is already having a default partition then
> adding a
> + new partition would result in an error if the default partition
> contains a
> + record that would fit in the new partition being added. This check
> is not
> + performed if the default partition is a foreign table.
> + </para>
>
> The indentation isn't correct here - it doesn't match the surrounding
> paragraphs. The bit about list or range partitioning doesn't match
> the actual behavior of the other patches, but maybe you intended this
> to document both this feature and what Beena's doing.
>
> I have tried to fix this now.

[1]
https://www.postgresql.org/message-id/CAOgcT0NwqnavYtu-QM-DAZ6N%3DwTiqKgy83WwtO2x94LSLZ1-Sw%40mail.gmail.com