Re: Adding support for Default partition in partitioning

Hi Ashutosh,

Please find my feedback inlined.

On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
> <jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
> > Hi,
> >
> > I have rebased the patches on the latest commit.
> >
>
> Thanks for rebasing the patches. The patches apply and compile
> cleanly. make check passes.
>
> Here are some review comments
> 0001 patch
> Most of this patch is same as 0002 patch posted in thread [1]. I have
> extensively reviewed that patch for Amit Langote. Can you please compare
> these
> two patches and try to address those comments OR just use patch from that
> thread? For example, canSkipPartConstraintValidation() is named as
> PartConstraintImpliedByRelConstraint() in that patch. OR
> + if (scanRel_constr == NULL)
> + return false;
> +
> is not there in that patch since returning false is wrong when
> partConstraint
> is NULL. I think this patch needs those fixes. Also, this patch set would
> need
> a rebase when 0001 from that thread gets committed.
>
>
I have renamed the canSkipPartConstraintValidation() to
PartConstraintImpliedByRelConstraint() and made other changes applicable per
Amit’s patch. This patch also refactors the scanning logic in
ATExecAttachPartition()
and adds it into a function ValidatePartitionConstraints(), hence I could
not use
Amit’s patch as it is. Please have a look into the new patch and let me
know if it
looks fine to you.

> 0002 patch
> + if (!and_args)
> + result = NULL;
> Add "NULL, if there are not partition constraints e.g. in case of default
> partition as the only partition.".

Added. Please check.

> This patch avoids calling
> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
> when
> partConstraint is NULL, but patches in [1] introduce more callers of
> canSkipPartConstraintValidation() which may pass NULL. So, it's better
> that we
> handle that case.
>

Following code added in patch 0001 now should take care of this.
+ num_check = (constr != NULL) ? constr->num_check : 0;

> 0003 patch
> + parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
> + heap_close(parentRel, NoLock);
>

As clarified earlier this was addressed in 0004 patch of V24 series. In
current set of patches this is now addressed in patch 0003 itself.

>
> + /*
> + * The default partition accepts any non-specified
> + * value, hence it should not get a mapped index
> while
> + * assigning those for non-null datums.
> + */
> Instead of "any non-specified value", you may want to use "any value not
> specified in the lists of other partitions" or something like that.
>

Changed the comment.

>
> + * If this is a NULL, route it to the null-accepting partition.
> + * Otherwise, route by searching the array of partition bounds.
> You may want to write it as "If this is a null partition key, ..." to
> clarify
> what's NULL.
>

Changed the comment.

>
> + * cur_index < 0 means we could not find a non-default partition
> of
> + * this parent. cur_index >= 0 means we either found the leaf
> + * partition, or the next parent to find a partition of.
> + *
> + * If we couldn't find a non-default partition check if the
> default
> + * partition exists, if it does, get its index.
> In order to avoid repeating "we couldn't find a ..."; you may want to add
> ",
> try default partition if one exists." in the first sentence itself.
>

Sorry, but I am not really sure how this change would make the comment
more readable than the current one.

> get_default_partition_oid() is defined in this patch and then redefined in
> 0004. Let's define it only once, mostly in or before 0003 patch.
>

get_default_partition_oid() is now defined only once in patch 0003.

>
> + * partition strategy. Assign the parent strategy to the default
> s/parent/parent's/
>

Fixed.

>
> +-- attaching default partition overlaps if the default partition already
> exists
> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
> +ERROR: cannot attach a new partition to table "list_parted" having a
> default partition
> For 0003 patch this testcase is same as the testcase in the next hunk; no
> new
> partition can be added after default partition. Please add this testcase in
> next set of patches.
>

Though the error message is same, the purpose of testing is different:
1. There cannot be more than one default partition,
2. and other is to test the fact the a new partition cannot be added if the
default partition exists.
The later test needs to be removed in next patch where we add support for
adding new partition even if a default partition exists.

> +-- fail
> +insert into part_default values ('aa', 2);
> May be explain why the insert should fail. "A row, which would fit
> other partition, does not fit default partition, even when inserted
> directly"
> or something like that. I see that many of the tests in that file do not
> explain why something should "fail" or be "ok", but may be it's better to
> document the reason for better readability and future reference.
>

Added a comment.

+-- check in case of multi-level default partitioned table
> s/in/the/ ?. Or you may want to reword it as "default partitioned
> partition in
> multi-level partitioned table" as there is nothing like "default
> partitioned
> table". May be we need a testcase where every level of a multi-level
> partitioned table has a default partition.
>
> I have changed the comment as well as added a test scenario where the
partition further has a default partition.

> +-- drop default, as we need to add some more partitions to test tuple
> routing
> Should be clubbed with the actual DROP statement?

This is needed in patch 0003, as it prevents adding/creating further
partitions
to parent. This is removed in patch 0004.

> +-- Check that addition or removal of any partition is correctly dealt
> with by
> +-- default partition table when it is being used in cached plan.
> Plan of a prepared statement gets cached only after it's executed 5 times.
> Before that the statement gets invalidated but there's not cached plan that
> gets invalidated. The test is fine here, but in order to test the cached
> plan
> as mentioned in the comment, you will need to execute the statement 5 times
> before executing drop statement. That's probably unnecessary, so just
> modify
> the comment to say "prepared statements instead of cached plan".
>

Agree. Fixed.

> 0004 patch
> The patch adds another column partdefid to catalog pg_partitioned_table.
> The
> column gives OID of the default partition for a given partitioned table.
> This
> means that the default partition's OID is stored at two places 1. in the
> default partition table's pg_class entry and in pg_partitioned_table.
> There is
> no way to detect when these two go out of sync. Keeping those two in sync
> is
> also a maintenance burdern. Given that default partition's OID is required
> only
> while adding/dropping a partition, which is a less frequent operation, it
> won't
> hurt to join a few catalogs (pg_inherits and pg_class in this case) to
> find out
> the default partition's OID. That will be occasional performance hit
> worth the otherwise maintenance burden.
>

To avoid partdefid of pg_partitioned_table going out of sync during any
future developments I have added an assert in RelationBuildPartitionDesc()
in patch 0003 in V25 series. I believe DBAs are not supposed to alter any
catalog tables, hence instead of adding an error, I added an Assert to
prevent
this breaking during development cycle.
We have similar kind of duplications in other catalogs e.g. pg_opfamily,
pg_operator etc. Also, per Robert [1], the other route of searching pg_class
and pg_inherits is going to cause some syscache bloat.

[1]
https://www.postgresql.org/message-id/CA%2BTgmobbnamyvii0pRdg9pp_jLHSUvq7u5SiRrVV0tEFFU58Tg%40mail.gmail.com

Regards,
Jeevan Ladhe