Re: [HACKERS] Binary search in fmgr_isbuiltin() is a bottleneck.
Hi Andres, Another idea would be to have an array of FmgrBuiltin*, that we index by > oid. That'd not be super small though, given that the space for function > oids is sparse. > > I totally agree here, as the oids are very much scattered having an array is not feasible here. > Thus what I've instead done is replacing the binary search in > fmgr_isbuiltin() with a simplehash.h style hashtable. After that the > lookup is still visible in the profile, but far less prominent. > > I'd like to move the hash creation out of fmgr_isbuiltin (to avoid > having to check whether it's already created), but there's currently no > convenient place to create the hash from. Now that we don't rely on > the sortedness of fmgrtab.c we could remove a few lines from > Gen_fmgrtab.pl, but I don't quite see the advantage. If we were > interested in a faster by-name lookup we could sort it by name, but > that'd be better solved by another hashtable... > I looked into these patches. Seems patch 004 is already committed, commit id: 791961f59b792fbd4f0a992d3ccab47298e79103 About patch 0005: The patch still applies cleanly. There are no failures in ‘make check’ + /* TODO: it'd be better if this were done separately */ + if (unlikely(oid2builtins == NULL)) { - int i = (high + low) / 2; - const FmgrBuiltin *ptr = &fmgr_builtins[i]; + int i; - if (id == ptr->foid) - return ptr; - else if (id > ptr->foid) - low = i + 1; - else - high = i - 1; + oid2builtins = oid2builtins_create(TopMemoryContext, + fmgr_nbuiltins, + NULL); + for (i = 0; i < fmgr_nbuiltins; i++) + { + const FmgrBuiltin *ptr = &fmgr_builtins[i]; + bool found; + + entry = oid2builtins_insert(oid2builtins, ptr->foid, &found); + Assert(!found); + entry->builtin = ptr; + } As Andres has already pointed, may be we want to move above code in a separate function, and just call that function here in case the hash is not already built. Further I am thinking about doing some performance testing, Andres can you please point me how did you test it and what perf numbers you saw for this particular patch(0005). Regards, Jeevan Ladhe
Re: [HACKERS] Optimise default partition scanning while adding new partition
Thanks Amit for reviewing. Patch looks fine to me. By the way, why don't we just say "Can we skip > scanning part_rel?" in the comment before the newly added call to > PartConstraintImpliedByRelConstraint()? We don't need to repeat the > explanation of what it does at the every place we call it. > I have changed the comment. PFA. Regards, Jeevan Ladhe 0001-Check-default-partitition-child-validation-scan-is.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Optimise default partition scanning while adding new partition
Hi, Commit 6f6b99d1335be8ea1b74581fc489a97b109dd08a introduced default partitioning support. This commit added a new function check_default_allows_bound(), which checks if there exists a row in the default partition that would belong to the new partition being added. If it finds one, it throws an error. Before taking the decision to scan the default partition, this function checks if there are existing constraints on default partition that would imply the new partition constraints, if yes it skips scanning the default partition, otherwise it scans the default partition and its children(if any). But, while doing so the current code misses the fact that there can be constraints on the child of default partition such that they would imply the constraints of the new partition being added, and hence individual child scan can also be skipped. Attached is the patch which does this. This is previously discussed in default partitioning thread[1], and decision was made that we can take this a separate patch rather than as a part of the default partitioning support. Amit Langote has a similar patch[2] for scanning the children of a partitioned table which is being attached as partition of another partitioned table. [1] https://www.postgresql.org/message-id/CA+TgmoZ8-q=2oahoxmvzbdnxi9g6i1idi4ozfkb67mk242d...@mail.gmail.com [2] https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3...@lab.ntt.co.jp Regards, Jeevan Ladhe 0001-Check-default-partitition-child-validation-scan-is.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
On Sat, Sep 9, 2017 at 3:05 AM, Robert Haas wrote: > On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe > wrote: > > Thanks Robert for taking care of this. > > My V29 patch series[1] is based on this commit now. > > Committed 0001-0003, 0005 with assorted modifications, mostly > cosmetic, but with some actual changes to describeOneTableDetails and > ATExecAttachPartition and minor additions to the regression tests. > > Thanks Robert!!
Re: [HACKERS] Adding support for Default partition in partitioning
On Fri, Sep 8, 2017 at 6:46 AM, Robert Haas wrote: > On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe > wrote: > > The fix would be much easier if the refactoring patch 0001 by Amul in > hash > > partitioning thread[2] is committed. > > Done. > Thanks Robert for taking care of this. My V29 patch series[1] is based on this commit now. [1] https://www.postgresql.org/message-id/CAOgcT0PCM5mJPCOyj3c0D1mLxwaVz6DJLO=nmz5j-5jgywx...@mail.gmail.com Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
On Thu, Sep 7, 2017 at 3:15 PM, Rajkumar Raghuwanshi < rajkumar.raghuwan...@enterprisedb.com> wrote: > On Wed, Sep 6, 2017 at 5:25 PM, Jeevan Ladhe < > jeevan.la...@enterprisedb.com> wrote: > >> Hi, >> >> Attached is the rebased set of patches. >> Robert has committed[1] patch 0001 in V26 series, hence the patch >> numbering >> in V27 is now decreased by 1 for each patch as compared to V26. >> > > Hi, > > I have applied v27 patches and while testing got below observation. > > Observation: in below partition table, d1 constraints not allowing NULL to > be inserted in b column > but I am able to insert it. > > steps to reproduce: > create table d0 (a int, b int) partition by range(a,b); > create table d1 partition of d0 for values from (0,0) to > (maxvalue,maxvalue); > > postgres=# insert into d0 values (0,null); > INSERT 0 1 > postgres=# \d+ d1 > Table "public.d1" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+ > -+--+- > a | integer | | | | plain > | | > b | integer | | | | plain > | | > Partition of: d0 FOR VALUES FROM (0, 0) TO (MAXVALUE, MAXVALUE) > Partition constraint: ((a IS NOT NULL) AND *(b IS NOT NULL) *AND ((a > 0) > OR ((a = 0) AND (b >= 0 > > postgres=# select tableoid::regclass,* from d0; > tableoid | a | b > --+---+--- > *d1 | 0 | * > (1 row) > Good catch. Thanks Rajkumar. This seems to be happening because of the following changes made in get_partition_for_tuple() for default range partition support as part of patch 0002. @@ -1971,27 +2204,10 @@ get_partition_for_tuple(PartitionDispatch *pd, ecxt->ecxt_scantuple = slot; FormPartitionKeyDatum(parent, slot, estate, values, isnull); - if (key->strategy == PARTITION_STRATEGY_RANGE) - { - /* - * Since we cannot route tuples with NULL partition keys through a - * range-partitioned table, simply return that no partition exists - */ - for (i = 0; i < key->partnatts; i++) - { - if (isnull[i]) - { - *failed_at = parent; - *failed_slot = slot; - result = -1; - goto error_exit; - } - } - } Instead of getting rid of this. If there isn't a default partition then we still do not have any range partition to route a null partition key and the routing should fail. I will work on a fix and send a patch shortly. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
lt partition. This is a merge of 0003 and 0004 > in > >> V24 series. > Comments below rather seem to be for the patch that extends default partition such that new partition can be added even when default partition exists. This is 0003 patch in V27. > > The commit message of this patch has following line, which no more applies > to > patch 0001. May be you want to remove this line or update the patch number. > 3. This patch uses the refactored functions created in patch 0001 > in this series. > Similarly the credit line refers to patch 0001. That too needs correction. > Fixed commit message. > > - * Also, invalidate the parent's relcache, so that the next rebuild will > load > - * the new partition's info into its partition descriptor. > + * Also, invalidate the parent's relcache entry, so that the next rebuild > will > + * load he new partition's info into its partition descriptor. If there > is a > + * default partition, we must invalidate its relcache entry as well. > Replacing "relcache" with "relcache entry" in the first sentence may be a > good > idea, but is unrelated to this patch. I would leave that change aside and > just > add comment about default partition. > Agree. Fixed. > +/* > + * The partition constraint for the default partition depends on the > + * partition bounds of every other partition, so we must invalidate > the > + * relcache entry for that partition every time a partition is added > or > + * removed. > + */ > +defaultPartOid = get_default_partition_oid(RelationGetRelid(parent)); > +if (OidIsValid(defaultPartOid)) > +CacheInvalidateRelcacheByRelid(defaultPartOid); > Again, since we have access to the parent's relcache, we should get the > default > partition OID from relcache rather than catalogs. > > This change is in heap.c, as I said above we would need to have a different version of get_default_partition_oid() to address this. Your thoughts? I haven't gone through the full patch yet, so there may be more > comments here. There is some duplication of code in > check_default_allows_bound() and ValidatePartitionConstraints() to > scan the children of partition being validated. The difference is that > the first one scans the relations in the same function and the second > adds them to work queue. May be we could use > ValidatePartitionConstraints() to scan the relation or add to the > queue based on some input flag may be wqueue argument itself. But I > haven't thought through this completely. Any thoughts? > check_default_allows_bound() is called only from DefineRelation(), and not for alter command. I am not really sure how can we use work queue for create command. [1] https://www.postgresql.org/message-id/CAOgcT0OM_Px6BXp1uDhhArw0bm-q4zCD5YwhDywR3K9ziBNL6A%40mail.gmail.com Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
On Sat, Sep 2, 2017 at 7:03 AM, Robert Haas wrote: > On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas wrote: > > On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe > > wrote: > >> 0001: > >> This patch refactors RelationBuildPartitionDesc(), basically this is > patch > >> 0001 of default range partition[1]. > > > > I spent a while studying this; it seems to be simpler and there's no > > real downside. So, committed. > > Thanks Robert for taking care of this. > BTW, the rest of this series seems to need a rebase. The changes to > insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3. Will rebase the patches. Regards, Jeevan Ladhe
Re: [HACKERS] Default Partition for Range
Hi Beena, On Tue, Aug 22, 2017 at 5:22 PM, Beena Emerson wrote: > On Tue, Aug 22, 2017 at 4:20 PM, Beena Emerson > wrote: > > Hi Jeevan, > > > > On Tue, Aug 22, 2017 at 7:53 AM, Jeevan Ladhe > > wrote: > >> > >> > >> 4. > >> static List * > >> -get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec) > >> +get_qual_for_range(Relation parent, PartitionBoundSpec *spec, > >> + bool for_default) > >> { > >> > >> The addition and the way flag ‘for_default’ is being used is very > confusing. > >> At this moment I am not able to think about a alternate solution to the > >> way you have used this flag. But will try and see if I can come up with > >> an alternate approach. > > > > Well, we need to skip the get_range_nulltest while collecting the qual > > of other partition to make one for default. We could skip this flag > > and remove the NullTest from the qual returned for each partition but > > I am not sure if thats a good way to go about it. > > > > > > Please find attached a patch which removes the for_default flag from > the get_qual_for_range function. This patch is just to show an idea > and should be applied over my earlier patch. There could be a better > way to do this. Let me know your opinion. > > + + foreach (lc, list_tmp) + { + Node *n = (Node *) lfirst(lc); + + if (IsA(n, NullTest)) + { + list_delete(part_qual, n); + count++; + } + } + I think its an unnecessary overhead to have the nulltest prepared first and then search for it and remove it from the partition qual. This is very ugly. I think the original idea is still better compared to this. May be we can rename the 'for_default' flag to something like 'part_of_default_qual'. Also, as discussed offline I will merge this with default list partitioning patch. Regards, Jeevan Ladhe
Re: [HACKERS] Default Partition for Range
Hi Beena, On Thu, Aug 17, 2017 at 4:59 PM, Beena Emerson wrote: > PFA the patch rebased over v25 patches of default list partition [1] > Thanks for rebasing. Range partition review: 1. There are lot of changes in RelationBuildPartitionDesc(). It was hard to understand why these changes are needed for default partition. I did not find any explanation for the same, may be I am missing some discussion? Only when I looked into it carefully I could understand that these changes are nothing but optimization for identifying the distinct bounds. I think it is better to keep the patch for code optimisation/simplification in a separate patch. I have separated the patch(0001) in attached patches for this purpose. 2. + * For default partition, it returns the negation of the constraints of all + * the other partitions. + * + * If we end up with an empty result list, we return NULL. We can rephrase the comment as below: "For default partition, function returns the negation of the constraints of all the other partitions. If default is the only partition then returns NULL." 3. @@ -2396,6 +2456,8 @@ make_one_range_bound(PartitionKey key, int index, List *datums, bool lower) ListCell *lc; int i; + Assert(datums != NULL); + bound = (PartitionRangeBound *) palloc0(sizeof(PartitionRangeBound)); bound->index = index; bound->datums = (Datum *) palloc0(key->partnatts * sizeof(Datum)); I am not really convinced, why should we have above Assert. 4. static List * -get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec) +get_qual_for_range(Relation parent, PartitionBoundSpec *spec, + bool for_default) { The addition and the way flag ‘for_default’ is being used is very confusing. At this moment I am not able to think about a alternate solution to the way you have used this flag. But will try and see if I can come up with an alternate approach. I still need to look at the test, and need to do some manual testing. Will update here with any findings. Patches: 0001: Refactoring/simplification of code in RelationBuildPartitionDesc(), 0002: implementation of default range partition by Beena. Regards, Jeevan 0001-Refactor-RelationBuildPartitionDesc.patch Description: Binary data 0002-Add-support-for-default-partition-for-range.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
On Fri, Aug 18, 2017 at 12:25 AM, Robert Haas wrote: > On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe > wrote: > > I have addressed following comments in V25 patch[1]. > > Committed 0001. Since that code seems to be changing about every 10 > minutes, it seems best to get this refactoring out of the way before > it changes again. Thanks Robert for taking care of this. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Robert, >> > 0007: > >> > This patch introduces code to check if the scanning of default > partition > >> > child > >> > can be skipped if it's constraints are proven. > >> > >> If I understand correctly, this is actually a completely separate > >> feature not intrinsically related to default partitioning. > > > > I don't see this as a new feature, since scanning the default partition > > will be introduced by this series of patches only, and rather than a > > feature this can be classified as a completeness of default skip > > validation logic. Your thoughts? > > Currently, when a partitioned table is attached, we check whether all > the scans can be checked but not whether scans on some partitions can > be attached. So there are two separate things: > > 1. When we introduce default partitioning, we need scan the default > partition either when (a) any partition is attached or (b) any > partition is created. > > 2. In any situation where scans are needed (scanning the partition > when it's attached, scanning the default partition when some other > partition is attached, scanning the default when a new partition is > created), we can run predicate_implied_by for each partition to see > whether the scan of that partition can be skipped. > > Those two changes are independent. We could do (1) without doing (2) > or (2) without doing (1) or we could do both. So they are separate > features. > > In my V25 series(patch 0005) I have only addressed half of (2) above i.e. code to check whether the scan of a partition of default partition can be skipped when other partition is being added. Amit Langote has submitted[1] a separate patch(0003) to address skipping the scan of the children of relation when it's being attached as a partition. [1] https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3869%40lab.ntt.co.jp Regards, Jeevan Ladhe
Re: [HACKERS] 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 NO INHERIT, the command will > fail; > - such a constraint must be recreated wit
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Ashutosh, On Thu, Aug 17, 2017 at 3:41 PM, Jeevan Ladhe wrote: > Hi Ashutosh, > > Please find my feedback inlined. > > On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat < > ashutosh.ba...@enterprisedb.com> wrote: > >> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe >> 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 readab
Re: [HACKERS] 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.ba...@enterprisedb.com> wrote: > On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe > 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
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Robert, > 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. I think even with this change there will be one level of indentation needed for throwing the error, as the error is to be thrown only if there exists a default partition. > > -* We must also lock the default partition, for the same > reasons explained > -* in heap_drop_with_catalog(). > +* We must lock the default partition, for the same reasons > explained in > +* DefineRelation(). > > I don't really see the point of this change. Whichever earlier patch > adds this code could include or omit the word "also" as appropriate, > and then this patch wouldn't need to change it. > > Actually the change is made because if the difference in the function name. I will remove ‘also’ from the first patch itself. > > 0007: > > This patch introduces code to check if the scanning of default partition > > child > > can be skipped if it's constraints are proven. > > If I understand correctly, this is actually a completely separate > feature not intrinsically related to default partitioning. I don't see this as a new feature, since scanning the default partition will be introduced by this series of patches only, and rather than a feature this can be classified as a completeness of default skip validation logic. Your thoughts? Regards, Jeevan Ladhe
Re: [HACKERS] Default Partition for Range
Hi Robert, Beena, On Wed, Aug 9, 2017 at 5:53 PM, Robert Haas wrote: > On Wed, Aug 9, 2017 at 8:18 AM, Rajkumar Raghuwanshi > wrote: > > --difference in the description of default partition in case of list vs > > range > > > > create table lp (a int) partition by list(a); > > create table lp_d partition of lp DEFAULT; > > postgres=# \d+ lp_d > >Table "public.lp_d" > > Column | Type | Collation | Nullable | Default | Storage | Stats > target > > | Description > > +-+---+--+-+ > -+--+- > > a | integer | | | | plain | > > | > > Partition of: lp DEFAULT > > Partition constraint: > > > > create table rp (a int) partition by range(a); > > create table rp_d partition of rp DEFAULT; > > postgres=# \d+ rp_d > >Table "public.rp_d" > > Column | Type | Collation | Nullable | Default | Storage | Stats > target > > | Description > > +-+---+--+-+ > -+--+- > > a | integer | | | | plain | > > | > > Partition of: rp DEFAULT > > Partition constraint: true > > This looks like a problem with the default list partitioning patch. I > think "true" is what we expect to see here in both cases. > In case of list partition if there is only default partition, then there is no specific value set that can be excluded from default partition, so in such a case DEFAULT partition for a list partitioned table simply will not have any constraint on it, and hence while the describe output is printed it does not have any constraints to be printed. Whereas, in case of default partition for a range partitioned table, in get_qual_for_range() it creates true boolean expression if default is the only partition. Here is the relevent code from Beena's patch: + else /* default is the only partition */ + result = list_make1(makeBoolConst(true, false)); I think, it is unnecessary to create a constraint that is going to be always true. Instead, if we have no constraints we can avoid some extra cpu cycles which would otherwise be wasted in evaluating an expression that is going to be always true. Having said this, I see a point that in such a case we should have some neat meaningful content to be printed for "Partition constraint: ". I think we can address this when we construct describe output string. Some ideas that come to my mind are: Partition constraint: NIL Partition constraint: no constraints No partition constraint. Partition constraint: true Please let me know your thoughts. Regards, Jeevan Ladhe
Re: [HACKERS] INSERT ON CONFLICT and partitioned tables
Thanks Amit for addressing the comment. The patch looks good to me. I have no more comments. Verified that v2 patch applies cleanly and make check passes. Thanks, Jeevan Ladhe
Re: [HACKERS] INSERT ON CONFLICT and partitioned tables
I applied the patch on latest master sources and the patch applies cleanly. The documentation is built without errors. We do not support following syntax for 'do nothing': postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b) do nothing; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification This limitation is because we do not support unique index on partitioned table. But, in that sense the following snippet of the documentation seems misleading: + will cause an error if the conflict target is specified (see +for more details). That means it's not + possible to specify DO UPDATE as the alternative + action, because it requires the conflict target to be specified. + On the other hand, specifying DO NOTHING as the + alternative action works fine. May be the last sentence can be rephrased as below: "On the other hand, specifying DO NOTHING without target as an alternative action works fine." Other than this patch looks good to me. Regards, Jeevan Ladhe On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote wrote: > Starting a new thread for a patch I posted earlier [1] to handle ON > CONFLICT DO NOTHING when inserting into a partitioned table. It's > intended for PG 11 and so registered in the upcoming CF. > > Summary of the previous discussion and the patch for anyone interested: > > Currently, if an INSERT statement for a partitioned table mentions the ON > CONFLICT clause, we error out immediately. It was implemented that way, > because it was thought that it could not be handled with zero support for > defining indexes on partitioned tables. Peter Geoghegan pointed out [2] > that it's too restrictive a view. > > He pointed out that planner doesn't *always* expect indexes to be present > on the table when ON CONFLICT is specified. They must be present though > if DO UPDATE action is requested, because one would need to also specify > the exact columns on which conflict will be checked and those must covered > by the appropriate indexes. So, if the table is partitioned and DO UPDATE > is specified, lack of indexes will result in an error saying that a > suitable index is absent. DO UPDATE action cannot be supported until we > implement the feature to define indexes on partitioned tables. > > OTOH, the DO NOTHING case should go through the planner without error, > because neither any columns need to be specified nor any indexes need to > be present covering them. So, DO NOTHING on partitioned tables might work > after all. Conflict can only be determined using indexes, which > partitioned tables don't allow, so how? Leaf partitions into which tuples > are ultimately stored can have indexes defined on them, which can be used > to check for the conflict. > > The patch's job is simple: > > - Remove the check in the parser that causes an error the moment the > ON CONFLICT clause is found. > > - Fix leaf partition ResultRelInfo initialization code so that the call > ExecOpenIndices() specifies 'true' for speculative, so that the > information necessary for conflict checking will be initialized in the > leaf partition's ResultRelInfo > > Thanks, > Amit > > [1] > https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb- > f5c8-a5b764ca96df%40lab.ntt.co.jp > > [2] > https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9- > JW3gRVPm1njLHck_w%40mail.gmail.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Ashutosh, 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(). > I think the patch 0004 exactly does what you have said here, i.e. it gets rid of the heap_open() and heap_close(). The question might be why I kept the patch 0004 a separate one, and the answer is I wanted to make it easier for review, and also keeping it that way would make it bit easy to work on a different approach if needed. About this: *"Also I don't see get_default_partition_oid() using Relation anywhere."* The get_default_partition_oid() uses parent relation to retrieve PartitionDesc from parent. Kindly let me know if you think I am still missing anything. Regards, Jeevan Ladhe
Re: [HACKERS] Default Partition for Range
Hi Beena, I have posted the rebased patches[1] for default list partition. Your patch also needs a rebase. [1] https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com Regards, Jeevan Ladhe On Fri, Jul 14, 2017 at 3:28 PM, Beena Emerson wrote: > On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed > wrote: > > > > Hello Beena, > > > > Thanks for the updated patch. It passes the basic tests which I > performed. > > > > Few comments: > > 1. In check_new_partition_bound(), > > > >> /* Default case is not handled here */ > >>if (spec->is_default) > >>break; > > The default partition check here can be performed in common for both > range > > and list partition. > > Removed the check here. > > > > > 2. RANGE_DATUM_FINITE = 0, /* actual datum stored elsewhere */ > > + RANGE_DATUM_DEFAULT,/* default */ > > > > More elaborative comment? > > I am not sure what to add. Do you have something in mind? > > > > > 3. Inside make_one_range_bound(), > > > >>+ > >>+ /* datums are NULL for default */ > >>+ if (datums == NULL) > >>+ for (i = 0; i < key->partnatts; i++) > >>+ bound->content[i] = RANGE_DATUM_DEFAULT; > >>+ > > IMO, returning bound at this stage will make code clearer as further > > processing > > does not happen for default partition. > > Done. > > > > > 4. > > @@ -549,6 +568,7 @@ RelationBuildPartitionDesc(Relation rel) > > sizeof(RangeDatumContent > > *)); > > boundinfo->indexes = (int *) palloc((ndatums + 1) * > > sizeof(int)); > > + boundinfo->default_index = -1; > > This should also be done commonly for both default list and range > partition. > > Removed the line here. Common allocation is done by Jeevan's patch. > > > > > 5. > > if (!spec->is_default && partdesc->nparts > 0) > > { > > ListCell *cell; > > > > Assert(boundinfo && > >boundinfo->strategy == > PARTITION_STRATEGY_LIST && > >(boundinfo->ndatums > 0 || > > partition_bound_accepts_nulls(boundinfo) || > > partition_bound_has_default(boundinfo))); > > The assertion condition partition_bound_has_default(boundinfo) is > rendered > > useless > > because of if condition change and can be removed perhaps. > > This cannot be removed. > This check is required when this code is run for a non-default > partition and default is the only existing partition. > > > > > 6. I think its better to have following regression tests coverage > > > > --Testing with inserting one NULL and other non NULL values for > multicolumn > > range partitioned table. > > > > postgres=# CREATE TABLE range_parted ( > > postgres(# a int, > > postgres(# b int > > postgres(# ) PARTITION BY RANGE (a, b); > > CREATE TABLE > > postgres=# > > postgres=# CREATE TABLE part1 ( > > postgres(# a int NOT NULL CHECK (a = 1), > > postgres(# b int NOT NULL CHECK (b >= 1 AND b <= 10) > > postgres(# ); > > CREATE TABLE > > > > postgres=# ALTER TABLE range_parted ATTACH PARTITION part1 FOR VALUES > FROM > > (1, 1) TO (1, 10); > > ALTER TABLE > > postgres=# CREATE TABLE partr_def1 PARTITION OF range_parted DEFAULT; > > CREATE TABLE > > > > postgres=# insert into range_parted values (1,NULL); > > INSERT 0 1 > > postgres=# insert into range_parted values (NULL,9); > > INSERT 0 1 > > added. > > > > > > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I have tried to address your comments in the V22 set of patches[1]. Please find my feedback inlined on your comments. On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Hello, I'd like to review this but it doesn't fit the master, as > Robert said. Especially the interface of predicate_implied_by is > changed by the suggested commit. > > Anyway I have some comment on this patch with fresh eyes. I > believe the basic design so my comment below are from a rather > micro viewpoint. > > - Considering on how canSkipPartConstraintValidation is called, I > *think* that RelationProvenValid() would be better. (But this > would be disappear by rebasing..) > > I think RelationProvenValid() is bit confusing in the sense that, it does not imply the meaning that some constraint is being checke > - 0002 changes the interface of get_qual_for_list, but left > get_qual_for_range alone. Anyway get_qual_for_range will have > to do the similar thing soon. > > Yes, this will be taken care with default partition for range. > - In check_new_partition_bound, "overlap" and "with" is > completely correlated with each other. "with > -1" means > "overlap = true". So overlap is not useless. ("with" would be > better to be "overlap_with" or somehting if we remove > "overlap") > > Agree, probably this can be taken as a separate refactoring patch. Currently, for in case of default I have got rid of "overlap", and now use of "with" and that is also used just for code simplification. > - The error message of check_default_allows_bound is below. > > "updated partition constraint for default partition \"%s\" >would be violated by some row" > > This looks an analog of validateCheckConstraint but as my > understanding this function is called only when new partition > is added. This would be difficult to recognize in the > situation. > > "the default partition contains rows that should be in >the new partition: \"%s\"" > > or something? > > I think the current error message is more clearer. Agree that there might be sort of confusion if it's due to addition or attach partition, but we have already stretched the message longer. I am open to suggestions here. > - In check_default_allows_bound, the iteration over partitions is > quite similar to what validateCheckConstraint does. Can we > somehow share validateCheckConstraint with this function? > > May be we can, but I think again this can also be categorized as refactoring patch and done later maybe. Your thoughts? > - In the same function, skipping RELKIND_PARTITIONED_TABLE is > okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem > good. I think at least some warning should be emitted. > > "Skipping foreign tables in the defalut partition. It might >contain rows that should be in the new partition." (Needs >preventing multple warnings in single call, maybe) > > Currently we do not emit any warning when attaching a foreign table as a non-default partition having rows that do not match its partition constraints and we still let attach the partition. But, I agree that we should emit such a warning, I added a code to do this. > - In the same function, the following condition seems somewhat > strange in comparison to validateCheckConstraint. > > > if (partqualstate && ExecCheck(partqualstate, econtext)) > > partqualstate won't be null as long as partition_constraint is > valid. Anyway (I'm believing that) an invalid constraint > results in error by ExecPrepareExpr. Therefore 'if > (partqualstate' is useless. > > Removed the check for partqualstate. > - In gram.y, the nonterminal for list spec clause is still > "ForValues". It seems somewhat strange. partition_spec or > something would be better. > > Done. Thanks for catching this, I agree with you. I have changed the name to PartitionBoundSpec. > - This is not a part of this patch, but in ruleutils.c, the error > for unknown paritioning strategy is emitted as following. > > > elog(ERROR, "unrecognized partition strategy: %d", > >(int) strategy); > > The cast is added because the strategy is a char. I suppose > this is because strategy can be an unprintable. I'd like to see > a comment if it is correct. > > I think this should be taken separately. Thanks, Jeevan Ladhe Refs: [1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X% 2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com
Re: [HACKERS] Adding support for Default partition in partitioning
ereport(ERROR, > +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION), > + errmsg("there exists a default partition for table > \"%s\", cannot attach a new partition", > +RelationGetRelationName(rel; > + > Should be done before heap_open on the table being attached. If we are not > going to attach the partition, there's no point in instantiating its > relcache. > Fixed. > > The comment in heap_drop_with_catalog() should mention why we lock the > default > partition before locking the table being dropped. > > extern List *preprune_pg_partitions(PlannerInfo *root, RangeTblEntry > *rte, > Index rti, Node *quals, LOCKMODE lockmode); > - > #endif /* PARTITION_H */ > Unnecessary hunk. I could not locate this hunk. Regards, Jeevan Ladhe Refs: [1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X% 2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com [2] https://www.postgresql.org/message-id/35d68d49-555f-421a-99f8-185a44d085a4%40lab.ntt.co.jp
Re: [HACKERS] Adding support for Default partition in partitioning
g attached has some existing row that violates partition constraints. Similarly, for consistency I have changed this in check_default_allows_bound() too. I agree that there is still a room for debate here after this change too, and also this change reverts the suggestion by Ashutosh. + * previously cached default partition constraints; those > constraints > + * won't stand correct after addition(or even removal) of a > partition. > > won't be correct after addition or removal > Done. > > + * allow any row that qualifies for this new partition. So, check > if > + * the existing data in the default partition satisfies this > *would be* > + * default partition constraint. > > check that the existing data in the default partition satisfies the > constraint as it will exist after adding this partition > Done. > > + * Need to take a lock on the default partition, refer comment for > locking > + * the default partition in DefineRelation(). > > I'd say: We must also lock the default partition, for the same reasons > explained in DefineRelation(). > > And similarly in the other places that refer to that same comment. > Done. > > +/* > + * In case of the default partition, the constraint is of the form > + * "!(result)" i.e. one of the following two forms: > + * 1. NOT ((keycol IS NULL) OR (keycol = ANY (arr))) > + * 2. NOT ((keycol IS NOT NULL) AND (keycol = ANY (arr))), where arr > is an > + * array of datums in boundinfo->datums. > + */ > > Does this survive pgindent? You might need to surround the comment > with dashes to preserve formatting. > > Yes, this din't survive pg_indent, but even adding dashes '--' did not make the deal(may be I misunderstood the workaround), I have instead added blank line in the bullets. I think it would be worth adding a little more text this comment, > something like this: Note that, in general, applying NOT to a > constraint expression doesn't necessarily invert the set of rows it > accepts, because NOT NULL is NULL. However, the partition constraints > we construct here never evaluate to NULL, so applying NOT works as > intended. > > Added. > + * Check whether default partition has a row that would fit the > partition > + * being attached by negating the partition constraint derived from > the > + * bounds. Since default partition is already part of the partitioned > + * table, we don't need to validate the constraints on the partitioned > + * table. > > Here again, I'd add to the end of the first sentence a parenthetical > note, like this: ...from the bounds (the partition constraint never > evaluates to NULL, so negating it like this is safe). > > Done. > I don't understand the second sentence. It seems to contradict the first > one. > Fixed, I removed the second sentence. > +extern List *get_default_part_validation_constraint(List > *new_part_constaints); > #endif /* PARTITION_H */ > > There should be a blank line after the last prototype and before the > #endif. > > +-- default partition table when it is being used in cahced plan. > > Typo. > Fixed. Thanks, Jeevan Ladhe Refs: [1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com [2] https://www.postgresql.org/message-id/CA%2BTgmobkFaptsmQiP94sbAKTtDKS6Azz%2BP4Bw1FxzmNrnyVa0w%40mail.gmail.com
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, On Mon, Jun 19, 2017 at 12:34 PM, Amit Langote < langote_amit...@lab.ntt.co.jp> wrote: > On 2017/06/16 14:16, Ashutosh Bapat wrote: > > On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas > wrote: > >> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat > >> wrote: > >>> Some more comments on the latest set of patches. > >> or looking up the OID in the > >> relcache multiple times. > > > > I am not able to understand this in the context of default partition. > > After that nobody else is going to change its partitions and their > > bounds (since both of those require heap_open on parent which would be > > stuck on the lock we hold.). So, we have to check only once if the > > table has a default partition. If it doesn't, it's not going to > > acquire one unless we release the lock on the parent i.e at the end of > > transaction. If it has one, it's not going to get dropped till the end > > of the transaction for the same reason. I don't see where we are > > looking up OIDs multiple times. > > Without heap_opening the parent, the only way is to look up parentOid's > children in pg_inherits and for each child looking up its pg_class tuple > in the syscache to see if its relpartbound indicates that it's a default > partition. That seems like it won't be inexpensive either. > > It would be nice if could get that information (that is - is a given > relation being heap_drop_with_catalog'd a partition of the parent that > happens to have default partition) in less number of steps than that. > Having that information in relcache is one way, but as mentioned, that > turns out be expensive. > > Has anyone considered the idea of putting the default partition OID in the > pg_partitioned_table catalog? Looking the above information up would > amount to one syscache lookup. Default partition seems to be special > enough object to receive a place in the pg_partitioned_table tuple of the > parent. Thoughts? > I liked this suggestion. Having an entry in pg_partitioned_table would avoid both expensive methods, i.e. 1. opening the parent or 2. lookup for each of the children first in pg_inherits and then its corresponding entry in pg_class. Unless anybody has any other suggestions/comments here, I am going to implement this suggestion. Thanks, Jeevan Ladhe
Re: [HACKERS] fix empty array expression in get_qual_for_list()
On Mon, Jun 26, 2017 at 8:14 PM, Tom Lane wrote: > Jeevan Ladhe writes: > > In case of list partitioned table: > > 1. If there is a partition accepting only null values and nothing else, > then > > currently the partition constraints for such a partition are constructed > as > > "((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))". > > I think there it is better to avoid constructing an empty array to avoid > > executing ANY expression. > > Looks like a good idea, pushed with trivial stylistic adjustments. > Thanks Tom for taking care of this. Regards, Jeevan Ladhe
Re: [HACKERS] fix empty array expression in get_qual_for_list()
Here is an example: create table t1 ( a int) partition by list (a); create table t11 partition of t1 for values in (null); *Current constraints:* postgres=# \d+ t11; Table "public.t11" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | | | plain | | Partition of: t1 FOR VALUES IN (NULL) Partition constraint: ((a IS NULL) OR (a = ANY (ARRAY[]::integer[]))) *Constraints after fix:* postgres=# \d+ t11; Table "public.t11" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | | | plain | | Partition of: t1 FOR VALUES IN (NULL) Partition constraint: (a IS NULL) Regards, Jeevan Ladhe On Mon, Jun 26, 2017 at 4:53 PM, Jeevan Ladhe wrote: > Hi, > > In case of list partitioned table: > 1. If there is a partition accepting only null values and nothing else, > then > currently the partition constraints for such a partition are constructed as > "((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))". > I think there it is better to avoid constructing an empty array to avoid > executing ANY expression. > > 2.Also, we are constructing an expression using index 0 of arrays in > PartitionKey since currently we have only one column for list partition in > key, > added an assert for this. > > PFA. > > Regards, > Jeevan Ladhe >
[HACKERS] fix empty array expression in get_qual_for_list()
Hi, In case of list partitioned table: 1. If there is a partition accepting only null values and nothing else, then currently the partition constraints for such a partition are constructed as "((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))". I think there it is better to avoid constructing an empty array to avoid executing ANY expression. 2.Also, we are constructing an expression using index 0 of arrays in PartitionKey since currently we have only one column for list partition in key, added an assert for this. PFA. Regards, Jeevan Ladhe fix_empty_arry_get_qual_for_list.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Thanks Ashutosh and Kyotaro for reviewing further. I shall address your comments in next version of my patch. Regards, Jeevan Ladhe On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Hello, I'd like to review this but it doesn't fit the master, as > Robert said. Especially the interface of predicate_implied_by is > changed by the suggested commit. > > Anyway I have some comment on this patch with fresh eyes. I > believe the basic design so my comment below are from a rather > micro viewpoint. > > At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote < > langote_amit...@lab.ntt.co.jp> wrote in 34ff801bf...@lab.ntt.co.jp> > > Oops, I meant to send one more comment. > > > > On 2017/06/15 15:48, Amit Langote wrote: > > > BTW, I noticed the following in 0002 > > + errmsg("there exists a default > partition for table \"%s\", cannot > > add a new partition", > > > > This error message style seems novel to me. I'm not sure about the best > > message text here, but maybe: "cannot add new partition to table \"%s\" > > with default partition" > > > > Note that the comment applies to both DefineRelation and > > ATExecAttachPartition. > > - Considering on how canSkipPartConstraintValidation is called, I > *think* that RelationProvenValid() would be better. (But this > would be disappear by rebasing..) > > - 0002 changes the interface of get_qual_for_list, but left > get_qual_for_range alone. Anyway get_qual_for_range will have > to do the similar thing soon. > > - In check_new_partition_bound, "overlap" and "with" is > completely correlated with each other. "with > -1" means > "overlap = true". So overlap is not useless. ("with" would be > better to be "overlap_with" or somehting if we remove > "overlap") > > - The error message of check_default_allows_bound is below. > > "updated partition constraint for default partition \"%s\" >would be violated by some row" > > This looks an analog of validateCheckConstraint but as my > understanding this function is called only when new partition > is added. This would be difficult to recognize in the > situation. > > "the default partition contains rows that should be in >the new partition: \"%s\"" > > or something? > > - In check_default_allows_bound, the iteration over partitions is > quite similar to what validateCheckConstraint does. Can we > somehow share validateCheckConstraint with this function? > > - In the same function, skipping RELKIND_PARTITIONED_TABLE is > okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem > good. I think at least some warning should be emitted. > > "Skipping foreign tables in the defalut partition. It might >contain rows that should be in the new partition." (Needs >preventing multple warnings in single call, maybe) > > - In the same function, the following condition seems somewhat > strange in comparison to validateCheckConstraint. > > > if (partqualstate && ExecCheck(partqualstate, econtext)) > > partqualstate won't be null as long as partition_constraint is > valid. Anyway (I'm believing that) an invalid constraint > results in error by ExecPrepareExpr. Therefore 'if > (partqualstate' is useless. > > - In gram.y, the nonterminal for list spec clause is still > "ForValues". It seems somewhat strange. partition_spec or > something would be better. > > - This is not a part of this patch, but in ruleutils.c, the error > for unknown paritioning strategy is emitted as following. > > > elog(ERROR, "unrecognized partition strategy: %d", > >(int) strategy); > > The cast is added because the strategy is a char. I suppose > this is because strategy can be an unprintable. I'd like to see > a comment if it is correct. > > > regards, > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Amit, On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote < langote_amit...@lab.ntt.co.jp> wrote: > Oops, I meant to send one more comment. > > On 2017/06/15 15:48, Amit Langote wrote: > > BTW, I noticed the following in 0002 > +errmsg("there exists a default > partition for table \"%s\", cannot > add a new partition", > > This error message style seems novel to me. I'm not sure about the best > message text here, but maybe: "cannot add new partition to table \"%s\" > with default partition" > This sounds confusing to me, what about something like: "\"%s\" has a default partition, cannot add a new partition." Note that this comment belongs to patch 0002, and it will go away in case we are going to have extended functionality i.e. patch 0003, as in that patch we allow user to create a new partition even in the cases when there exists a default partition. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, Sorry for being away from here. I had some issues with my laptop, and I have resumed working on this. On Thu, Jun 15, 2017 at 1:21 AM, Robert Haas wrote: > On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe > wrote: > > Here are the details of the patches in attached zip. > > 0001. refactoring existing ATExecAttachPartition code so that it can be > > used for > > default partitioning as well > > 0002. support for default partition with the restriction of preventing > > addition > > of any new partition after default partition. > > 0003. extend default partitioning support to allow addition of new > > partitions. > > 0004. extend default partitioning validation code to reuse the refactored > > code > > in patch 0001. > > I think the core ideas of this patch are pretty solid now. It's come > a long way in the last month. High-level comments: > Thanks Robert for looking into this. > - Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55. > Will rebase. > - Still no documentation. > - Should probably be merged with the patch to add default partitioning > for ranges. > Will try to get this soon. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
While rebasing the current set of patches to the latest source, I realized that it might be a good idea to split the default partitioning support patch further into two incremental patches, where the first patch for default partition support would prevent addition of any new partition if there exists a default partition, and then an incremental patch which allows to create/attach a new partition even if there exists a default partition provided the default partition does not have any rows satisfying the bounds of the new partition being added. This would be easier for review. Here are the details of the patches in attached zip. 0001. refactoring existing ATExecAttachPartition code so that it can be used for default partitioning as well 0002. support for default partition with the restriction of preventing addition of any new partition after default partition. 0003. extend default partitioning support to allow addition of new partitions. 0004. extend default partitioning validation code to reuse the refactored code in patch 0001. PFA Regards, Jeevan Ladhe On Mon, Jun 12, 2017 at 11:49 AM, Jeevan Ladhe < jeevan.la...@enterprisedb.com> wrote: > > On Mon, Jun 12, 2017 at 9:39 AM, Ashutosh Bapat < > ashutosh.ba...@enterprisedb.com> wrote: > >> While the refactoring seems a reasonable way to re-use existing code, >> that may change based on the discussion in [1]. Till then please keep >> the refactoring patches separate from the main patch. In the final >> version, I think the refactoring changes to ATAttachPartition and the >> default partition support should be committed separately. So, please >> provide three different patches. That also makes review easy. >> > > Sure Ashutosh, > > PFA. > default_partition_splits_v21.tar.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix possible optimizations in ATExecAttachPartition()
On Wed, Jun 14, 2017 at 2:12 AM, Robert Haas wrote: > On Tue, Jun 13, 2017 at 5:22 AM, Amit Langote > wrote: > > Yeah, I was thinking the same while writing the patch posted on the > thread > > "A bug in mapping attributes in ATExecAttachPartition()" [1]. That patch > > adds the break you mention in 2, but didn't do anything about point 1. > > > > In any case, +1 to your patch. I'll rebase if someone decides to commit > > it first. > > If the patch I posted in > https://www.postgresql.org/message-id/CA%2BTgmoYmW9VwCWDpe7eXUxeKmAKOxm > g8itgFkB5UTQTq4SnTjQ%40mail.gmail.com > gets committed, all of this code will be gone entirely, so this will > be moot. If we decide to repair the existing broken logic rather than > ripping it out entirely then this is probably a good idea, but I hope > that's not what happens. > That seems a better option to me too. +1 Regards, Jeevan Ladhe
[HACKERS] fix possible optimizations in ATExecAttachPartition()
Hi, I was doing some testing for my default partitioning work, and I realized that there seem to be a some optimization possible (and I think we should really have this optimization) for logic around handling the "key IS NOT NULL" constraints followed by predicate_implied_by() call. 1. Consider, if predicate_implied_by() returns false, in that case skip_validate is left set to false, and we really do not want to do lots of stuff followed this decision just to prove that the scan cannot be avoided(which is already decided in this case) if there is no NOT NULL constraint on the partition key column. 2. Further, if we have already decided not to skip the scan, we should really have a break in following if block: if (!partition_accepts_null && (partattno == 0 || !bms_is_member(partattno, not_null_attrs))) skip_validate = false; I have made above changes in the attached patch. PFA, and let me know if I am missing something here. Regards, Jeevan Ladhe fix_atexecattahpartition.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
On Mon, Jun 12, 2017 at 9:39 AM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > While the refactoring seems a reasonable way to re-use existing code, > that may change based on the discussion in [1]. Till then please keep > the refactoring patches separate from the main patch. In the final > version, I think the refactoring changes to ATAttachPartition and the > default partition support should be committed separately. So, please > provide three different patches. That also makes review easy. > Sure Ashutosh, PFA. default_partition_v20_patches.tar.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
default partition or a child of default partition(which I dint like much, but I don't see a way out here). Still the error message does not display the default partition name in error as of check_default_allows_bound(). May be to address this and keep the messages exactly similar we can copy the name of parent default partition in a field in AlteredTableInfo structure, which looks very ugly to me. I am open to suggestions here. *3. changes to default_partition_v19.patch:* The default partition constraint are no more built using the negator of the operator, instead it is formed simply as NOT of the existing partitions: e.g.: if a null accepting partition already exists: NOT ((keycol IS NULL) OR (keycol = ANY (arr))) if a null accepting partition does not exists: NOT ((keycol IS NOT NULL) AND (keycol = ANY (arr))), where arr is an array of datums in boundinfo->datums. Added tests for prepared statment. Renamed RelationGetDefaultPartitionOid() to get_default_partition_oid(). + if (partqualstate && ExecCheck(partqualstate, econtext)) + ereport(ERROR, + (errcode(ERRCODE_CHECK_VIOLATION), + errmsg("new partition constraint for default partition \"%s\" would be violated by some row", + RelationGetRelationName(default_rel; Per Ashutosh's suggestion[2], changed the error code to ERRCODE_INVALID_OBJECT_DEFINITION. Also, per Robert's suggestion[3], changed following message: "new partition constraint for default partition \"%s\" would be violated by some row" to "updated partition constraint for default partition \"%s\" would be violated by some row" Some other cosmetic changes. Apart from this, I am exploring the tests in relation with NOT NULL constraint embedded within an expression. Will update on that shortly. [1] http://www.postgresql-archive.org/A-bug-in-mapping-attributes-in-ATExecAttachPartition-td5965298.html [2] http://www.postgresql-archive.org/Adding-support-for-Default-partition-in-partitioning-td5946868i120.html#a5965277 [3] http://www.postgresql-archive.org/Adding-support-for-Default-partition-in-partitioning-tp5946868p5965599.html Regards, Jeevan Ladhe On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe > wrote: > > > > >> > >> The code in check_default_allows_bound() to check whether the default > >> partition > >> has any rows that would fit new partition looks quite similar to the > code > >> in > >> ATExecAttachPartition() checking whether all rows in the table being > >> attached > >> as a partition fit the partition bounds. One thing that > >> check_default_allows_bound() misses is, if there's already a constraint > on > >> the > >> default partition refutes the partition constraint on the new partition, > >> we can > >> skip the scan of the default partition since it can not have rows that > >> would > >> fit the new partition. ATExecAttachPartition() has code to deal with a > >> similar > >> case i.e. the table being attached has a constraint which implies the > >> partition > >> constraint. There may be more cases which check_default_allows_bound() > >> does not > >> handle but ATExecAttachPartition() handles. So, I am wondering whether > >> it's > >> better to somehow take out the common code into a function and use it. > We > >> will > >> have to deal with a difference through. The first one would throw an > error > >> when > >> finding a row that satisfies partition constraints whereas the second > one > >> would > >> throw an error when it doesn't find such a row. But this difference can > be > >> handled through a flag or by negating the constraint. This would also > take > >> care > >> of Amit Langote's complaint about foreign partitions. There's also > another > >> difference that the ATExecAttachPartition() queues the table for scan > and > >> the > >> actual scan takes place in ATRewriteTable(), but there is not such queue > >> while > >> creating a table as a partition. But we should check if we can reuse the > >> code to > >> scan the heap for checking a constraint. > >> > >> In case of ATTACH PARTITION, probably we should schedule scan of default > >> partition in the alter table's work queue like what > >> ATExecAttachPartition() is > >> doing for the table being attached. That would fit in the way alter > table > >> works. > > > > I tried refactoring existing code so that it can be used for defaul
Re: [HACKERS] Adding support for Default partition in partitioning
Thanks Ashutosh, On Thu, Jun 8, 2017 at 4:04 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat > wrote: > > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe > > wrote: > > > >> > >>> > >>> The code in check_default_allows_bound() to check whether the default > >>> partition > >>> has any rows that would fit new partition looks quite similar to the > code > >>> in > >>> ATExecAttachPartition() checking whether all rows in the table being > >>> attached > >>> as a partition fit the partition bounds. One thing that > >>> check_default_allows_bound() misses is, if there's already a > constraint on > >>> the > >>> default partition refutes the partition constraint on the new > partition, > >>> we can > >>> skip the scan of the default partition since it can not have rows that > >>> would > >>> fit the new partition. ATExecAttachPartition() has code to deal with a > >>> similar > >>> case i.e. the table being attached has a constraint which implies the > >>> partition > >>> constraint. There may be more cases which check_default_allows_bound() > >>> does not > >>> handle but ATExecAttachPartition() handles. So, I am wondering whether > >>> it's > >>> better to somehow take out the common code into a function and use it. > We > >>> will > >>> have to deal with a difference through. The first one would throw an > error > >>> when > >>> finding a row that satisfies partition constraints whereas the second > one > >>> would > >>> throw an error when it doesn't find such a row. But this difference > can be > >>> handled through a flag or by negating the constraint. This would also > take > >>> care > >>> of Amit Langote's complaint about foreign partitions. There's also > another > >>> difference that the ATExecAttachPartition() queues the table for scan > and > >>> the > >>> actual scan takes place in ATRewriteTable(), but there is not such > queue > >>> while > >>> creating a table as a partition. But we should check if we can reuse > the > >>> code to > >>> scan the heap for checking a constraint. > >>> > >>> In case of ATTACH PARTITION, probably we should schedule scan of > default > >>> partition in the alter table's work queue like what > >>> ATExecAttachPartition() is > >>> doing for the table being attached. That would fit in the way alter > table > >>> works. > >> > > > > I tried refactoring existing code so that it can be used for default > > partitioning as well. The code to validate the partition constraints > > against the table being attached in ATExecAttachPartition() is > > extracted out into a set of functions. For default partition we reuse > > those functions to check whether it contains any row that would fit > > the partition being attached. While creating a new partition, the > > function to skip validation is reused but the scan portion is > > duplicated from ATRewriteTable since we are not in ALTER TABLE > > context. The names of the functions, their declaration will require > > some thought. > > > > There's one test failing because for ATTACH partition the error comes > > from ATRewriteTable instead of check_default_allows_bounds(). May be > > we want to use same message in both places or some make ATRewriteTable > > give a different message while validating default partition. > > > > Please review the patch and let me know if the changes look good. > > From the discussion on thread [1], that having a NOT NULL constraint > embedded within an expression may cause a scan to be skipped when it > shouldn't be. For default partitions such a case may arise. If an > existing partition accepts NULL and we try to attach a default > partition, it would get a NOT NULL partition constraint but it will be > buried within an expression like !(key = any(array[1, 2, 3]) OR key is > null) where the existing partition/s accept values 1, 2, 3 and null. > We need to check whether the refactored code handles this case > correctly. v19 patch does not have this problem since it doesn't try > to skip the scan based on the constraints of the table being attached. > Please try following cases 1. a default partition accepting nulls > exists and we attach a partition to accept NULL values 2. a NULL > accepting partition exists and we try to attach a table as default > partition. In both the cases default partition should be checked for > rows with NULL partition keys. In both the cases, if the default > partition table has a NOT NULL constraint we should be able to skip > the scan and should scan the table when such a constraint does not > exist. > I will review your refactoring patch as well test above cases. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
IIUC, default partition constraints is simply NOT IN ( other sibling partitions>). > If constraint on the default partition refutes the new partition's > constraints that means we have overlapping partition, and perhaps > error. > You are correct Amul, but this error will be thrown before we try to check for the default partition data. So, in such cases I think we really do not need to have logic to check if default partition refutes the new partition contraints. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
gt; Not applicable now, as I removed the later part of the comment. } > + > ; > unnecessary extra line. > Removed. > +/* > + * The default partition bound does not have any datums to be > + * transformed, return the new bound. > + */ > Probably not needed. > Removed. > > +if (spec->is_default && (strategy == > PARTITION_STRATEGY_LIST || > + strategy == > PARTITION_STRATEGY_RANGE)) > +{ > +appendStringInfoString(buf, "DEFAULT"); > +break; > +} > + > What happens if strategy is something other than RANGE or LIST. For that > matter > why not just LIST? Possibly you could write this as > +if (spec->is_default) > +{ > +Assert(strategy == PARTITION_STRATEGY_LIST); > +appendStringInfoString(buf, "DEFAULT"); > +break; > +} > Done. > @@ -2044,7 +2044,7 @@ psql_completion(const char *text, int start, int end) > COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, ""); > /* Limited completion support for partition bound specification */ > else if (TailMatches3("ATTACH", "PARTITION", MatchAny)) > -COMPLETE_WITH_CONST("FOR VALUES"); > +COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT"); > else if (TailMatches2("FOR", "VALUES")) > COMPLETE_WITH_LIST2("FROM (", "IN ("); > > @@ -2483,7 +2483,7 @@ psql_completion(const char *text, int start, int end) > COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables, > ""); > /* Limited completion support for partition bound specification */ > else if (TailMatches3("PARTITION", "OF", MatchAny)) > -COMPLETE_WITH_CONST("FOR VALUES"); > +COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT"); > Do we include psql tab completion in the main feature patch? I have not > seen > this earlier. But appreciate taking care of these defails. > I am not sure about this. If needed I can submit a patch to take care of this later, but as of now I have not removed this from the patch. +char *ExecBuildSlotValueDescription(Oid reloid, > needs an "extern" declaration. > Per one of the comment[1] given by Amit Langote, I have removed a call to ExecBuildSlotValueDescription(), and this was a leftover, I cleaned it up. [1]https://www.postgresql.org/message-id/7c758a6b-107e-7c82- 0d3c-3af7965cad3f%40lab.ntt.co.jp Regards, Jeevan Ladhe default_partition_v19.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
> What is the reason the new patch does not mention of violating rows > when a new partition overlaps with default? > Is it because more than one row could be violating the condition? > This is because, for reporting the violating error, I had to function ExecBuildSlotValueDescription() public. Per Amit's comment I have removed this change and let the overlapping error without row contains. I think this is analogus to other functions that are throwing violation error but are not local to execMain.c. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Robert, Thanks for your comments: > If DETACH PARTITION and DROP PARTITION require this, why not ATTACH > PARTITION and CREATE TABLE .. PARTITION OF? > > For CREATE and ATTACH parition the invalidation of default relation is taken care by the following clean-up part in check_default_allows_bound(): + ResetExprContext(econtext); + CHECK_FOR_INTERRUPTS(); + } + + CacheInvalidateRelcache(part_rel); + MemoryContextSwitchTo(oldCxt); However, post your comment I carefully looked in the code I wrote here, and I see that this still explicitly needs cache invalidation in ATTACH and CREATE command, because the above invalidation call will not happen in case the default partition is further partitioned. Plus, I think the call to CacheInvalidateRelcache() in check_default_allows_bound() can be completely removed. This code however will be rearranged, as I plan to address Ashutosh's one of the comment to write a function for common code of ATExecAttachPartition() and check_default_allows_bound(). Regards, Jeevan Ladhe
Re: [HACKERS] BEFORE trigger can cause undetected partition constraint violation
I tried to debug this, and I see that while the target partition index is correctly found in ExecInsert(), somehow the resultRelInfo->ri_PartitionCheck is NIL, this is extracted from array mstate->mt_partitions. This prevents execution of constraints in following code snippet in ExecInsert(): /* * Check the constraints of the tuple */ if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck) ExecConstraints(resultRelInfo, slot, estate); I couldn't debug it further today. Regards, Jeevan Ladhe On Fri, Jun 2, 2017 at 1:21 AM, Robert Haas wrote: > I just discovered that a BEFORE trigger can allow data into a > partition that violates the relevant partition constraint. This is > bad. > > Here is an example: > > rhaas=# create or replace function t() returns trigger as $$begin > new.a := 2; return new; end$$ language plpgsql; > CREATE FUNCTION > rhaas=# create table foo (a int, b text) partition by list (a); > CREATE TABLE > rhaas=# create table foo1 partition of foo for values in (1); > CREATE TABLE > rhaas=# create trigger x before insert on foo1 for each row execute > procedure t(); > CREATE TRIGGER > rhaas=# insert into foo values (1, 'hi there'); > INSERT 0 1 > rhaas=# select tableoid::regclass, * from foo; > tableoid | a |b > --+---+-- > foo1 | 2 | hi there > (1 row) > > That row violates the partition constraint, which requires that a = 1. > You can see that by trying to insert the same row into the partition > directly: > > rhaas=# insert into foo1 values (2, 'hi there'); > ERROR: new row for relation "foo1" violates partition constraint > DETAIL: Failing row contains (2, hi there). > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I have addressed Ashutosh's and Amit's comments in the attached patch. Please let me know if I have missed anything and any further comments. PFA. Regards, Jeevan Ladhe On Wed, May 31, 2017 at 9:50 AM, Beena Emerson wrote: > On Wed, May 31, 2017 at 8:13 AM, Amit Langote > wrote: > > On 2017/05/31 9:33, Amit Langote wrote: > > > > > > In get_rule_expr(): > > > > case PARTITION_STRATEGY_LIST: > > Assert(spec->listdatums != NIL); > > > > +/* > > + * If the boundspec is of Default partition, it > does > > + * not have list of datums, but has only one > node to > > + * indicate its a default partition. > > + */ > > +if (isDefaultPartitionBound( > > +(Node *) > linitial(spec->listdatums))) > > +{ > > +appendStringInfoString(buf, "DEFAULT"); > > +break; > > +} > > + > > > > How about adding this part before the switch (key->strategy)? That way, > > we won't have to come back and add this again when we add range default > > partitions. > > I think it is best that we add a bool is_default to PartitionBoundSpec > and then have a general check for both list and range. Though > listdatums, upperdatums and lowerdatums are set to default for a > DEFAULt partition, it does not seem proper that we check listdatums > for range as well. > > > > > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > default_partition_v18.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Thanks Amit for your comments. On 31-May-2017 6:03 AM, "Amit Langote" wrote: Hi Jeevan, On 2017/05/30 16:38, Jeevan Ladhe wrote: > I have rebased the patch on the latest commit. > PFA. Was looking at the patch and felt that the parse node representation of default partition bound could be slightly different. Can you explain the motivation behind implementing it without adding a new member to the PartitionBoundSpec struct? I would suggest instead adding a bool named is_default and be done with it. It will help get rid of the public isDefaultPartitionBound() in the proposed patch whose interface isn't quite clear and instead simply check if (spec->is_default) in places where it's called by passing it (Node *) linitial(spec->listdatums). I thought of reusing the existing members of PartitionBoundSpec, but I agree that having a bool could simplify the code. Will do the receptive change. Further looking into the patch, I found a tiny problem in check_default_allows_bound(). If the default partition that will be scanned by it is a foreign table or a partitioned table with a foreign leaf partition, you will get a failure like: -- default partition is a foreign table alter table p attach partition fp default; -- adding a new partition will try to scan fp above alter table p attach partition p12 for values in (1, 2); ERROR: could not open file "base/13158/16456": No such file or directory I think the foreign tables should be ignored here to avoid the error. The fact that foreign default partition may contain data that satisfies the new partition's constraint is something we cannot do much about. Also, see the note in ATTACH PARTITION description regarding foreign tables [1] and the discussion at [2]. Will look into this. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I have fixed the issue related to default partition constraints not getting updated after detaching a partition. PFA. Regards, Jeevan Ladhe On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe wrote: > Hi, > > I have rebased the patch on the latest commit. > PFA. > > There exists one issue reported by Rajkumar[1] off-line as following, where > describing the default partition after deleting null partition, does not > show > updated constraints. I am working on fixing this issue. > > create table t1 (c1 int) partition by list (c1); > create table t11 partition of t1 for values in (1,2); > create table t12 partition of t1 default; > create table t13 partition of t1 for values in (10,11); > create table t14 partition of t1 for values in (null); > > postgres=# \d+ t12 > Table "public.t12" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+ > -+--+- > c1 | integer | | | | plain | > | > Partition of: t1 DEFAULT > Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10, > 11]))) > > postgres=# alter table t1 detach partition t14; > ALTER TABLE > postgres=# \d+ t12 > Table "public.t12" > Column | Type | Collation | Nullable | Default | Storage | Stats > target | Description > +-+---+--+-+ > -+--+- > c1 | integer | | | | plain | > | > Partition of: t1 DEFAULT > Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10, > 11]))) > > postgres=# insert into t1 values(null); > INSERT 0 1 > > Note that the parent correctly allows the nulls to be inserted. > > [1] rajkumar.raghuwan...@enterprisedb.com > > Regards, > Jeevan Ladhe > > On Tue, May 30, 2017 at 10:59 AM, Beena Emerson > wrote: > >> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe >> wrote: >> > Hi, >> > >> > I have rebased the patch on latest commit with few cosmetic changes. >> > >> > The patch fix_listdatums_get_qual_for_list_v3.patch [1] needs to be >> applied >> > before applying this patch. >> > >> > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg >> 315490.html >> > >> >> >> This needs a rebase again. >> >> -- >> >> Beena Emerson >> >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > default_partition_v17.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I have rebased the patch on the latest commit. PFA. There exists one issue reported by Rajkumar[1] off-line as following, where describing the default partition after deleting null partition, does not show updated constraints. I am working on fixing this issue. create table t1 (c1 int) partition by list (c1); create table t11 partition of t1 for values in (1,2); create table t12 partition of t1 default; create table t13 partition of t1 for values in (10,11); create table t14 partition of t1 for values in (null); postgres=# \d+ t12 Table "public.t12" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- c1 | integer | | | | plain | | Partition of: t1 DEFAULT Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10, 11]))) postgres=# alter table t1 detach partition t14; ALTER TABLE postgres=# \d+ t12 Table "public.t12" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- c1 | integer | | | | plain | | Partition of: t1 DEFAULT Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10, 11]))) postgres=# insert into t1 values(null); INSERT 0 1 Note that the parent correctly allows the nulls to be inserted. [1] rajkumar.raghuwan...@enterprisedb.com Regards, Jeevan Ladhe On Tue, May 30, 2017 at 10:59 AM, Beena Emerson wrote: > On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe > wrote: > > Hi, > > > > I have rebased the patch on latest commit with few cosmetic changes. > > > > The patch fix_listdatums_get_qual_for_list_v3.patch [1] needs to be > applied > > before applying this patch. > > > > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/ > msg315490.html > > > > > This needs a rebase again. > > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > default_partition_v16.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix side-effect in get_qual_for_list()
On Mon, May 29, 2017 at 11:55 PM, Tom Lane wrote: > Jeevan Ladhe writes: > > I have rebased the patch on recent commit. > > Pushed with some further tweaking. > Thanks Tom for taking care of this. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I have rebased the patch on latest commit with few cosmetic changes. The patch fix_listdatums_get_qual_for_list_v3.patch [1] <http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html> needs to be applied before applying this patch. [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html Regards, Jeevan Ladhe On Mon, May 29, 2017 at 2:28 PM, Jeevan Ladhe wrote: > > >> The existing comment is not valid >> /* >> * A null partition key is only acceptable if null-accepting >> list >> * partition exists. >> */ >> as we allow NULL to be stored in default. It should be updated. >> > > Sure Beena, as stated earlier will update this on my next version of patch. > > > Regards, > Jeevan Ladhe > default_partition_v15.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix side-effect in get_qual_for_list()
Hi, I have rebased the patch on recent commit. With recent commits, some of the hunks in the v2 patch related to castNode, are not needed. PFA. Regards, Jeevan Ladhe On Sat, May 27, 2017 at 1:16 AM, Jeevan Ladhe wrote: > Hi Ashutosh, > > Thanks for catching this. For now this isn't a problem since >> generate_partition_qual() is crafting PartitionBoundInfo which it >> doesn't use anywhere else. But if the function gets used where the >> PartitionBoundSpec is being used somewhere else as well. > > > Yes, this behavior currently does not affect adversely, but I think this > function is quite useful for future enhancements and should be fixed. > > While you are >> at it, can we use castNode() in place of >> PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you >> think it should be done separately? >> > > I have made this change at couple of places applicable. > > PFA. > > Regards, > Jeevan Ladhe > fix_listdatums_get_qual_for_list_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Partition for Range
Hi Beena, I went through your patch, and here are some of my comments: - For generating a qual for default range partition, instead of scanning for all the children and collecting all the boundspecs, how about creating and negating an expression from the lists of lowerdatums and upperdatums in boundinfo. - Wrong comment: + int default_index; /* Index of the default list partition. */ - Suggested by Robert earlier on default list partitioning thread, instead of abbreviating is_def/found_def use is(found)_default etc. - unrelated change: - List *all_parts; + List *all_parts; - typo: partiton should be partition, similar typo at other places too. + * A non-default range partiton table does not currently allow partition keys - Useless hunk for this patch: - Oiddefid; + Oid defid; - better to use IsA here, instead of doing explicit check: + bound->content[i] = (datum->type == T_DefElem) + ? RANGE_DATUM_DEFAULT - It is better to use head of either lowerdatums or upperdatums list to verify if its a default partition and get rid of the constant PARTITION_DEFAULT altogether. + { + /* + * If the partition is the default partition switch back to + * PARTITION_STRATEGY_RANGE + */ + if (spec->strategy == PARTITION_DEFAULT) + { + is_def = true; + result_spec->strategy = PARTITION_STRATEGY_RANGE; + } - I am sorry, but I could not understand following hunk. Does this change really belongs to this patch? If not, it will be better to handle it separately. @@ -2242,33 +2387,16 @@ get_partition_for_tuple(PartitionDispatch *pd, ecxt->ecxt_scantuple = slot; FormPartitionKeyDatum(parent, slot, estate, values, isnull); - if (key->strategy == PARTITION_STRATEGY_RANGE) + if (key->strategy == PARTITION_STRATEGY_LIST && isnull[0]) { /* - * Since we cannot route tuples with NULL partition keys through - * a range-partitioned table, simply return that no partition - * exists + * A null partition key is only acceptable if null-accepting list + * partition exists. */ - for (i = 0; i < key->partnatts; i++) - { - if (isnull[i]) - { - *failed_at = parent; - *failed_slot = slot; - result = -1; - goto error_exit; - } - } + if (partition_bound_accepts_nulls(partdesc->boundinfo)) + cur_index = partdesc->boundinfo->null_index; - Change not related to this patch: - List *all_parts; + List *all_parts; - In the function get_qual_from_partbound() is_def is always going to be false for range partition, the function get_qual_for_range can be directly passed false instead. - Following comment for function get_qual_for_range_default() implies that this function returns bool, but the actually it returns a List. + * + * If DEFAULT is the only partiton for the table then this returns TRUE. + * Regards, Jeevan Ladhe On Wed, May 24, 2017 at 12:52 AM, Beena Emerson wrote: > Hello, > > On Mon, May 22, 2017 at 11:29 AM, Ashutosh Bapat < > ashutosh.ba...@enterprisedb.com> wrote: > >> On Mon, May 22, 2017 at 7:27 AM, Beena Emerson >> wrote: >> Would it be more readable if this reads as NOT >> (constraint_for_partition_1 || constraint_for_partition_2 || >> constraint_for_partition_3)? That way, the code to create >> constraint_for_partition_n can be reused and there's high chance that >> we will end up with consistent constraints? >> > > PFA the patch which gives the default partition constraint as you have > suggested. > > >> >> > >> > It still needs more work: >> > 1. Handling addition of new partition after default, insertion of data, >> few >> > more bugs >> > 2. Documentation >> > 3. Regression tests >> > >> >> I think, the default partition support for all the strategies >> supporting it should be a single patch since most of the code will be >> shared? >> >> > Dependency on list default patch: > gram.y : adding the syntax > partition.c: > - default_index member in PartitionBoundInfoData; > - check_new_partition_bound : the code for adding a partition after > default has been completely reused. > - isDefaultPartitionBound function is used. > > The structures are same but the handling of list and range is very > different and the code mostly has the switch case construct to handle the > 2 separately. I feel it could be taken separately. > > As suggested in the default list thread, I have created > a partition_bound_has_default macro and avoided usage of has_default in > range code. This has to be used for list as well. > Another suggestion for list which has to be implemented in this patch in > removal of PARTITION_DEFAULT. Ii have not done this in this version. > > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
Re: [HACKERS] Adding support for Default partition in partitioning
> > The existing comment is not valid > /* > * A null partition key is only acceptable if null-accepting > list > * partition exists. > */ > as we allow NULL to be stored in default. It should be updated. > Sure Beena, as stated earlier will update this on my next version of patch. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
This patch needs a rebase on recent commits, and also a fix[1] that is posted for get_qual_for_list(). I am working on both of these tasks. Will update the patch once I am done with this. Regards, Jeevan Ladhe On Mon, May 29, 2017 at 12:25 PM, Beena Emerson wrote: > On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe > wrote: > > > > Forgot to attach the patch. > > PFA. > > > > On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe < > jeevan.la...@enterprisedb.com> wrote: > >> > >> Hi Rajkumar, > >> > >>> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST > (a); > >>> CREATE TABLE > >>> postgres=# CREATE TEMP TABLE temp_def_part (a int); > >>> CREATE TABLE > >>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part > DEFAULT; > >>> server closed the connection unexpectedly > >>> This probably means the server terminated abnormally > >>> before or while processing the request. > >>> The connection to the server was lost. Attempting reset: Failed. > >>> !> > >> > >> > >> Thanks for reporting. > >> PFA patch that fixes above issue. > >> > > > The existing comment is not valid > /* > * A null partition key is only acceptable if null-accepting > list > * partition exists. > */ > as we allow NULL to be stored in default. It should be updated. > > DROP TABLE list1; > CREATE TABLE list1 (a int) PARTITION BY LIST (a); > CREATE TABLE list1_1 (LIKE list1); > ALTER TABLE list1 ATTACH PARTITION list1_1 FOR VALUES IN (2); > CREATE TABLE list1_def PARTITION OF list1 DEFAULT; > INSERT INTO list1 VALUES (NULL); > SELECT * FROM list1_def; > a > --- > > (1 row) > > > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] fix side-effect in get_qual_for_list()
Hi Ashutosh, Thanks for catching this. For now this isn't a problem since > generate_partition_qual() is crafting PartitionBoundInfo which it > doesn't use anywhere else. But if the function gets used where the > PartitionBoundSpec is being used somewhere else as well. Yes, this behavior currently does not affect adversely, but I think this function is quite useful for future enhancements and should be fixed. While you are > at it, can we use castNode() in place of > PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you > think it should be done separately? > I have made this change at couple of places applicable. PFA. Regards, Jeevan Ladhe fix_listdatums_get_qual_for_list_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] fix side-effect in get_qual_for_list()
Hi, While working on one of the crash reported on default partition for list partitioning table[1] I found some strange behavior in get_qual_for_list() while I tried to call it from the new code I wrote for default partition. After debugging, I noticed that the function get_qual_for_list() is implicitly manipulating the (PartitionBoundSpec) spec->listdatums list. AFAICU, this manipulation is needed just to construct a list of datums to be passed to ArrayExpr, and this should be done without manipulating the original list. The function name is get_qual_for_list(), which implies that this function returns something and does not modify anything. I have made this change in attached patch, as I think this is useful for future developments, as there may be a need in future to call get_qual_for_list() from other places, and the caller might not expect that PartitionBoundSpec gets modified. PFA. [1] *https://www.postgresql.org/message-id/flat/CAOgcT0PLPge%3D5U6%3DGU5SnC3_8yutCbWWOiUva3Cw94M9zpbvgQ%40mail.gmail.com <https://www.postgresql.org/message-id/flat/CAOgcT0PLPge%3D5U6%3DGU5SnC3_8yutCbWWOiUva3Cw94M9zpbvgQ%40mail.gmail.com>* Regards, Jeevan Ladhe fix_listdatums_get_qual_for_list.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Forgot to attach the patch. PFA. On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe wrote: > Hi Rajkumar, > > postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a); >> CREATE TABLE >> postgres=# CREATE TEMP TABLE temp_def_part (a int); >> CREATE TABLE >> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part >> DEFAULT; >> server closed the connection unexpectedly >> This probably means the server terminated abnormally >> before or while processing the request. >> The connection to the server was lost. Attempting reset: Failed. >> !> >> > > Thanks for reporting. > PFA patch that fixes above issue. > > Regards, > Jeevan Ladhe > default_partition_v14.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rajkumar, postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a); > CREATE TABLE > postgres=# CREATE TEMP TABLE temp_def_part (a int); > CREATE TABLE > postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part > DEFAULT; > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > !> > Thanks for reporting. PFA patch that fixes above issue. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi, I started looking into Rahila's default_partition_v11.patch, and reworked on few things as below: - I tried to cover all the review comments posted on the thread. Do let me know if something is missing. - Got rid of the functions get_qual_for_default() and generate_qual_for_defaultpart(). There is no need of collecting boundspecs of all the partitions in case of list partition, the list is available in boundinfo->ndatums, an expression for default can be created from the information that is available in boundinfo. - Got rid of variable has_default, and added a macro for it. - Changed the logic of checking the overlapping of existing rows in default partition. Earlier version of patch used to build new constraints for default partition table and then was checking if any of existing rows violate those constraints. However, current version of patch just checks if any of the rows in default partition satisfy the new partition's constraint and fail if there exists any. This logic can also be used as it is for default partition in case of RANGE partitioning. - Simplified grammar rule. - Got rid of PARTITION_DEFAULT since DEFAULT is not a different partition strategy, the applicable logic is also revised: - There are few other code adjustments like: indentation, commenting, code simplification etc. - Added regression tests. TODO: Documentation, I am working on it. Will updated the patch soon. PFA. Regards, Jeevan On Mon, May 22, 2017 at 7:31 AM, Beena Emerson wrote: > Hello, > > Patch for default range partition has been added. PFA the rebased v12 > patch for the same. > I have not removed the has_default variable yet. > > Default range partition: https://www.postgresql.org/message-id/ > CAOG9ApEYj34fWMcvBMBQ-YtqR9fTdXhdN82QEKG0SVZ6zeL1xg%40mail.gmail.com > -- > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > default_partition_v13.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] unreachable code in partition.c
Hi, Following code in function get_qual_for_list(partition.c) is not reachable. else result = list_make1(opexpr); Attached is the patch that removes this dead code. Regards, Jeevan Ladhe partition_remove_dead_code.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] fix hard-coded index in make_partition_op_expr
On Thu, May 18, 2017 at 12:13 AM, Robert Haas wrote: > > Agreed. Committed your patch. > Thanks Robert!
Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData
> I committed this with fixes for those issues, plus I renamed the macro > to partition_bound_accepts_nulls, which I think is more clear. Thanks Robert.
[HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData
Hi, As discussed in default partition thread[1] <https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com>, here is the patch to remove has_null from PartitionBoundInfoData structure. Basically flag has_null is not needed and null_index can be checked if the current bound is having a null value or not. For simplicity of future use, in attached patch I have introduced a macro that would return TRUE if the given bound has null. PFA. [1]. https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com Regards, Jeevan Ladhe remove_has_null_PartitionBoundInfoData_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] fix hard-coded index in make_partition_op_expr
Hi, While browsing through the partitioning code, I noticed that a recent commit f8bffe9e6d700fd34759a92e47930ce9ba7dcbd5, which fixes multi-column range partitioning constraints, introduced a function make_partition_op_expr, that takes keynum as a input parameter to identify the index of the partition key. In case of range partition we can have multiple partition keys but for list partition we have only one. Considering that, I think following code does not cause any side-effect logically(and may be a oversight while moving the code from function get_qual_for_list to this function): saopexpr->inputcollid = key->partcollation[0]; saopexpr->args = list_make2(arg1, arg2); But as we have keynum now, should we be using it to index key->partcollation, instead of a hard coded '0'. I tried to look around in list partitioning and hard coded '0' is used at all the places, but that code is either list specific or does not have availability of partitioned key index. Attached patch does this small change in make_partition_op_expr. Another way is to, have an Assert in case of PARTITION_STRATEGY_LIST: Assert(keynum != 0); PFA. Regards, Jeevan Ladhe fix_indexing_make_partition_op_expr_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
On Wed, May 17, 2017 at 2:28 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Tue, May 16, 2017 at 9:01 PM, Robert Haas > wrote: > > On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe > > wrote: > >> I have fixed the crash in attached patch. > >> Also the patch needed bit of adjustments due to recent commit. > >> I have re-based the patch on latest commit. > > > > +boolhas_default;/* Is there a default partition? > > Currently false > > + * for a range partitioned table */ > > +intdefault_index;/* Index of the default list > > partition. -1 for > > + * range partitioned tables */ > > > > We have has_null and null_index for list partitioning. There > null_index == -1 = has_null. May be Rahila and/or Jeevan just copied > that style. Probably we should change that as well? > > I agree with Ashutosh. I had given similar comment on earlier version of patch[1] <https://www.postgresql.org/message-id/CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA%40mail.gmail.com>, and Rahila reverted with above reasoning, hence did not change the logic she introduced. Probably its a good idea to have a separate patch that removes has_null logic, in a separate thread. [1] https://www.postgresql.org/message-id/CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA%40mail.gmail.com Regards, Jeevan Ladhe.
Re: [HACKERS] Adding support for Default partition in partitioning
On Fri, May 12, 2017 at 7:34 PM, Beena Emerson wrote: > > Thank you for the updated patch. However, now I cannot create a partition > after default. > > CREATE TABLE list1 ( > a int, > b int > ) PARTITION BY LIST (a); > > CREATE TABLE list1_1 (LIKE list1); > ALTER TABLE list1 ATTACH PARTITION list1_1 FOR VALUES IN (1); > CREATE TABLE list1_def PARTITION OF list1 DEFAULT; > CREATE TABLE list1_5 PARTITION OF list1 FOR VALUES IN (3); > > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > !> > Hi, I have fixed the crash in attached patch. Also the patch needed bit of adjustments due to recent commit. I have re-based the patch on latest commit. PFA. Regards, Jeevan Ladhe default_partition_v12.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, On Thu, May 11, 2017 at 7:37 PM, Rahila Syed wrote: > > >3. > >In following function isDefaultPartitionBound, first statement "return > false" > >is not needed. > It is needed to return false if the node is not DefElem. > Please have a look at following code: + * Returns true if the partition bound is default + */ +bool +isDefaultPartitionBound(Node *value) +{ + if (IsA(value, DefElem)) + { + DefElem defvalue = (DefElem ) value; + if(!strcmp(defvalue->defname, "DEFAULT")) + return true; + return false; + } + return false; +} By first return false, I mean to say the return statement inside the if block "if (IsA(value, DefElem))": + if(!strcmp(defvalue->defname, "DEFAULT")) + return true; + return false; Even if this "return false" is not present, the control is anyway going to fall through and will return false from the outermost return statement. I leave this decision to you, but further this block could be rewritten as below and also can be defined as a macro: bool isDefaultPartitionBound(Node *value) { return (IsA(value, DefElem) && !strcmp(((DefElem) value)->defname, "DEFAULT")); } Regards, Jeevan Ladhe
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > We add PARTITION OF clause for a table which is partition, so if the > parent is not present while restoring, the restore is going to fail. +1 But, similarly for inheritance if we dump a child table, it's dump is broken as of today. When we dump a child table we append "inherits(parenttab)" clause at the end of the DDL. Later when we try to restore this table on a database not having the parenttab, the restore fails. So, I consider this as a bug. Consider following example: postgres=# create table tab1(a int, b int); CREATE TABLE postgres=# create table tab2(c int, d char) inherits(tab1); CREATE TABLE postgres=# \! pg_dump postgres -t tab2 > a.sql postgres=# create database bkdb; CREATE DATABASE postgres=# \! psql bkdb < a.sql SET SET SET SET SET SET SET SET SET SET SET ERROR: relation "tab1" does not exist ERROR: relation "tab2" does not exist ERROR: relation "tab2" does not exist invalid command \. postgres=# Regards, Jeevan Ladhe
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi Amit, Ashutosh, On Tue, May 9, 2017 at 3:29 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Tue, May 9, 2017 at 3:13 PM, Amit Langote > wrote: > > On 2017/05/09 17:21, Jeevan Ladhe wrote: > >> On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > >>> Current pg_dump --exclude-table option excludes partitioned relation > >>> and dumps all its child relations and vice versa for --table option, > which > >>> I think is incorrect. > >>> > >>> In this case we might need to explore all partitions and exclude or > include > >>> from dump according to given pg_dump option, attaching WIP patch > proposing > >>> same fix. Thoughts/Comments? > >> > >> Also, I can see similar issue exists with inheritance. > >> In attached patch, I have extended Amul's original patch to address the > >> inheritance dumping issue. > > > > Perhaps, it will be better not to touch the regular inheritance tables in > > this patch. > > Yeah, I think it's fine if parent gets dumped without one or more of > its children, that's user's choice when it used a certain pattern. > Problematic case might be when we dump a child without its parent and > have INHERITS clause there. pg_restore would throw an error. But in > case that problem exists it's very old and should be fixed separately. I agree that this should be taken as a separate fix, rather than taking it with partition. Regards, Jeevan Ladhe
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
In my last email by mistake I attached Amul's patch itself. Please find attached patch extending the fix to inheritance relations. Regards, Jeevan Ladhe On Tue, May 9, 2017 at 1:51 PM, Jeevan Ladhe wrote: > Hi, > > On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > >> Hi, >> >> Current pg_dump --exclude-table option excludes partitioned relation >> and dumps all its child relations and vice versa for --table option, which >> I think is incorrect. >> >> In this case we might need to explore all partitions and exclude or >> include >> from dump according to given pg_dump option, attaching WIP patch proposing >> same fix. Thoughts/Comments? >> >> Regards, >> Amul >> > > +1. > > Also, I can see similar issue exists with inheritance. > In attached patch, I have extended Amul's original patch to address the > inheritance dumping issue. > > Regards, > Jeevan Ladhe > > pg_dump_fix_for_partition_and_inheritance_wip.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.
Hi, On Tue, May 9, 2017 at 12:43 PM, amul sul wrote: > Hi, > > Current pg_dump --exclude-table option excludes partitioned relation > and dumps all its child relations and vice versa for --table option, which > I think is incorrect. > > In this case we might need to explore all partitions and exclude or include > from dump according to given pg_dump option, attaching WIP patch proposing > same fix. Thoughts/Comments? > > Regards, > Amul > +1. Also, I can see similar issue exists with inheritance. In attached patch, I have extended Amul's original patch to address the inheritance dumping issue. Regards, Jeevan Ladhe pg_dump_fix_for_partition_and_inheritance.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Robert, Thanks for your explnation. On Mon, May 8, 2017 at 9:56 PM, Robert Haas wrote: > On Thu, May 4, 2017 at 4:28 PM, Jeevan Ladhe > wrote: > > While reviewing the code I was trying to explore more cases, and I here > > comes an > > open question to my mind: > > should we allow the default partition table to be partitioned further? > > I think yes. In general, you are allowed to partition a partition, > and I can't see any justification for restricting that for default > partitions when we allow it everywhere else. > > > If we allow it(as in the current case) then observe following case, > where I > > have defined a default partitioned which is further partitioned on a > > different > > column. > > > > postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST > (a); > > CREATE TABLE > > postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, > 7, > > 8); > > CREATE TABLE > > postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY > > LIST(b); > > CREATE TABLE > > postgres=# INSERT INTO test VALUES (20, 24, 12); > > ERROR: no partition of relation "test_pd" found for row > > DETAIL: Partition key of the failing row contains (b) = (24). > > > > Note, that it does not allow inserting the tuple(20, 24, 12) because > though > > a=20 > > would fall in default partition i.e. test_pd, table test_pd itself is > > further > > partitioned and does not have any partition satisfying b=24. > > Right, that looks like correct behavior. You would have gotten the > same result if you had tried to insert into test_pd directly. > > > Further if I define a default partition for table test_pd, the the tuple > > gets inserted. > > That also sounds correct. > > > Doesn't this sound like the whole purpose of having DEFAULT partition on > > test > > table is defeated? > > Not to me. It's possible to do lots of silly things with partitioned > tables. For example, one case that we talked about before is that you > can define a range partition for, say, VALUES (0) TO (100), and then > subpartition it and give the subpartitions bounds which are outside > the range 0-100. That's obviously silly and will lead to failures > inserting tuples, but we chose not to try to prohibit it because it's > not really broken, just useless. There are lots of similar cases > involving other features. For example, you can apply an inherited > CHECK (false) constraint to a table, which makes it impossible for > that table or any of its children to ever contain any rows; that is > probably a dumb configuration. You can create two unique indexes with > exactly the same definition; unless you're creating a new one with the > intent of dropping the old one, that doesn't make sense. You can > define a trigger that always throws an ERROR and then another trigger > which runs later that modifies the tuple; the second will never be run > because the first one will always kill the transaction before we get > there. Those things are all legal, but often unuseful. Similarly > here. Defining a default list partition and then subpartitioning it > by list is not likely to be a good schema design, but it doesn't mean > we should try to disallow it. It is important to distinguish between > things that are actually *broken* (like a partitioning configuration > where the tuples that can be inserted into a partition manually differ > from the ones that are routed to it automatically) and things that are > merely *lame* (like creating a multi-level partitioning hierarchy when > a single level would have done the job just as well). The former > should be prevented by the code, while the latter is at most a > documentation issue. I agree with you that it is a user perspective on how he decides to do partitions of already partitioned table, and also we should have a demarcation between things to be handled by code and things to be left as common-sense or ability to define a good schema. I am ok with current behavior, provided we have atleast one-lineer in documentation alerting the user that partitioning the default partition will limit the ability of routing the tuples that do not fit in any other partitions. Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, I am not able add a new partition if default partition is further partitioned with default partition. Consider example below: postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a); CREATE TABLE postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7, 8); CREATE TABLE postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY LIST(b); CREATE TABLE postgres=# CREATE TABLE test_pd_pd PARTITION OF test_pd DEFAULT; CREATE TABLE postgres=# INSERT INTO test VALUES (20, 24, 12); INSERT 0 1 *postgres=# CREATE TABLE test_p2 PARTITION OF test FOR VALUES IN(15);* *ERROR: could not open file "base/12335/16420": No such file or directory* Thanks, Jeevan Ladhe On Fri, May 5, 2017 at 11:55 AM, Rajkumar Raghuwanshi < rajkumar.raghuwan...@enterprisedb.com> wrote: > Hi Rahila, > > pg_restore is failing for default partition, dump file still storing old > syntax of default partition. > > create table lpd (a int, b int, c varchar) partition by list(a); > create table lpd_d partition of lpd DEFAULT; > > create database bkp owner 'edb'; > grant all on DATABASE bkp to edb; > > --take plain dump of existing database > \! ./pg_dump -f lpd_test.sql -Fp -d postgres > > --restore plain backup to new database bkp > \! ./psql -f lpd_test.sql -d bkp > > psql:lpd_test.sql:63: ERROR: syntax error at or near "DEFAULT" > LINE 2: FOR VALUES IN (DEFAULT); >^ > > > vi lpd_test.sql > > -- > -- Name: lpd; Type: TABLE; Schema: public; Owner: edb > -- > > CREATE TABLE lpd ( > a integer, > b integer, > c character varying > ) > PARTITION BY LIST (a); > > > ALTER TABLE lpd OWNER TO edb; > > -- > -- Name: lpd_d; Type: TABLE; Schema: public; Owner: edb > -- > > CREATE TABLE lpd_d PARTITION OF lpd > FOR VALUES IN (DEFAULT); > > > ALTER TABLE lpd_d OWNER TO edb; > > > Thanks, > Rajkumar >
Re: [HACKERS] Adding support for Default partition in partitioning
While reviewing the code I was trying to explore more cases, and I here comes an open question to my mind: should we allow the default partition table to be partitioned further? If we allow it(as in the current case) then observe following case, where I have defined a default partitioned which is further partitioned on a different column. postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a); CREATE TABLE postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7, 8); CREATE TABLE postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY LIST(b); CREATE TABLE postgres=# INSERT INTO test VALUES (20, 24, 12); ERROR: no partition of relation "test_pd" found for row DETAIL: Partition key of the failing row contains (b) = (24). Note, that it does not allow inserting the tuple(20, 24, 12) because though a=20 would fall in default partition i.e. test_pd, table test_pd itself is further partitioned and does not have any partition satisfying b=24. Further if I define a default partition for table test_pd, the the tuple gets inserted. Doesn't this sound like the whole purpose of having DEFAULT partition on test table is defeated? Any views? Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, I have started reviewing your latest patch, and here are my initial comments: 1. In following block, we can just do with def_index, and we do not need found_def flag. We can check if def_index is -1 or not to decide if default partition is present. @@ -166,6 +172,8 @@ RelationBuildPartitionDesc(Relation rel) /* List partitioning specific */ PartitionListValue **all_values = NULL; bool found_null = false; + bool found_def = false; + int def_index = -1; int null_index = -1; 2. In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove following duplicate declaration of boundinfo, because it is confusing and after your changes it is not needed as its not getting overridden in the if block locally. if (partdesc->nparts > 0) { PartitionBoundInfo boundinfo = partdesc->boundinfo; ListCell *cell; 3. In following function isDefaultPartitionBound, first statement "return false" is not needed. + * Returns true if the partition bound is default + */ +bool +isDefaultPartitionBound(Node *value) +{ + if (IsA(value, DefElem)) + { + DefElem *defvalue = (DefElem *) value; + if(!strcmp(defvalue->defname, "DEFAULT")) + return true; + return false; + } + return false; +} 4. As mentioned in my previous set of comments, following if block inside a loop in get_qual_for_default needs a break: + foreach(cell1, bspec->listdatums) + { + Node *value = lfirst(cell1); + if (isDefaultPartitionBound(value)) + { + def_elem = true; + *defid = inhrelid; + } + } 5. In the grammar the rule default_part_list is not needed: +default_partition: + DEFAULT { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); } + +default_part_list: + default_partition { $$ = list_make1($1); } + ; + Instead you can simply declare default_partition as a list and write it as: default_partition: DEFAULT { Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1); $$ = list_make1(def); } 6. You need to change the output of the describe command, which is currently as below: postgres=# \d+ test; Table "public.test" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +-+---+--+-+-+--+- a | integer | | | | plain | | b | date | | | | plain | | Partition key: LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4, 5) What about changing the Paritions output as below: Partitions: *pd DEFAULT,* test_p1 FOR VALUES IN (4, 5) 7. You need to handle tab completion for DEFAULT. e.g. If I partially type following command: CREATE TABLE pd PARTITION OF test DEFA and then press tab, I get following completion: CREATE TABLE pd PARTITION OF test FOR VALUES I did some primary testing and did not find any problem so far. I will review and test further and let you know my comments. Regards, Jeevan Ladhe On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi < rajkumar.raghuwan...@enterprisedb.com> wrote: > On Thu, May 4, 2017 at 5:14 PM, Rahila Syed > wrote: > >> The syntax implemented in this patch is as follows, >> >> CREATE TABLE p11 PARTITION OF p1 DEFAULT; >> >> Applied v9 patches, table description still showing old pattern of > default partition. Is it expected? > > create table lpd (a int, b int, c varchar) partition by list(a); > create table lpd_d partition of lpd DEFAULT; > > \d+ lpd > Table "public.lpd" > Column | Type| Collation | Nullable | Default | Storage | > Stats target | Description > +---+---+--+ > -+--+--+- > a | integer | | | | plain > | | > b | integer | | | | plain > | | > c | character varying | | | | extended > | | > Partition key: LIST (a) > Partitions: lpd_d FOR VALUES IN (DEFAULT) >
Re: [HACKERS] warning in twophase.c
I have also been seeing this warning lately, I had tried to check git log, and seems like following commit 546c13e11b29 by Simon has introduced it. But per the commit log, this seems to be a temporary arrangement: commit 546c13e11b29a5408b9d6a6e3cca301380b47f7f Author: Simon Riggs Date: Sun Apr 23 22:12:01 2017 +0100 Workaround for RecoverPreparedTransactions() Force overwriteOK = true while we investigate deeper fix Proposed by Tom Lane as temporary measure, accepted by me With latest update on following thread, it seems like Simon has proposed a fix that gets rid of flag overwriteOK: http://www.postgresql-archive.org/StandbyRecoverPreparedTransactions-recovers-subtrans-links-incorrectly-td5957751.html#a5957853 Regards, Jeevan Ladhe On Tue, Apr 25, 2017 at 6:36 AM, Amit Langote wrote: > Been seeing this warning recently: > > twophase.c: In function ‘RecoverPreparedTransactions’: > twophase.c:1916:9: warning: variable ‘overwriteOK’ set but not used > [-Wunused-but-set-variable] >bool overwriteOK = false; > ^~~ > > As the message says, the value of overwriteOK is not used anywhere in > RecoverPreparedTransactions: > > booloverwriteOK = false; > > /* > * It's possible that SubTransSetParent has been set before, if > * the prepared transaction generated xid assignment records. Test > * here must match one used in AssignTransactionId(). > */ > if (InHotStandby && (hdr->nsubxacts >= PGPROC_MAX_CACHED_SUBXIDS || > XLogLogicalInfoActive())) > overwriteOK = true; > > Couldn't we get rid of it? > > Thanks, > Amit > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] DELETE and UPDATE with LIMIT and ORDER BY
Hi Surafel, IIUC, the requirement of the feature also had one of the consideration where one needs to delete large data and that takes long time, and adding LIMIT should reduce the overhead by allowing to delete the data in batches. I did a quick performance test, and in following example you can see the conventional delete taking "355.288 ms" VS "1137.248 ms" with new LIMIT syntax. postgres=# create table mytab(a int, b varchar(50)); CREATE TABLE postgres=# insert into mytab(a, b) select i,md5(random()::text) from generate_series(1, 100) s(i); INSERT 0 100 postgres=# \timing Timing is on. postgres=# delete from mytab order by a limit 20 offset 0; DELETE 20 *Time: 1137.248 ms (00:01.137)* postgres=# truncate mytab; TRUNCATE TABLE Time: 21.717 ms postgres=# insert into mytab(a, b) select i,md5(random()::text) from generate_series(1, 100) s(i); INSERT 0 100 Time: 3166.445 ms (00:03.166) postgres=# delete from mytab where a < 21; DELETE 20 *Time: 355.288 ms* Am I missing something here? Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas > wrote: > > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed > wrote: > >> Following can also be considered as it specifies more clearly that the > >> partition holds default values. > >> > >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT; > > > > Yes, that could be done. But I don't think it's correct to say that > > the partition holds default values. Let's back up and ask what the > > word "default" means. The relevant definition (according to Google or > > whoever they stole it from) is: > > > > a preselected option adopted by a computer program or other mechanism > > when no alternative is specified by the user or programmer. > > > > So, a default *value* is the value that is used when no alternative is > > specified by the user or programmer. We have that concept, but it's > > not what we're talking about here: that's configured by applying the > > DEFAULT property to a column. Here, we're talking about the default > > *partition*, or in other words the *partition* that is used when no > > alternative is specified by the user or programmer. So, that's why I > > proposed the syntax I did. The partition doesn't contain default > > values; it is itself a default. > > Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more > natural. > +1
Re: [HACKERS] Adding support for Default partition in partitioning
partition constraint" 10. Additionally, I did test your given sample test in first post and the one mentioned by Keith; both of them are passing without errors. Also, I did a pg_dump test and it is dumping the partitions and data correctly. But as mentioned earlier, it would be good if you have them in your patch. I will do further review and let you know comments if any. Regards, Jeevan Ladhe On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas > wrote: > > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed > wrote: > >> Following can also be considered as it specifies more clearly that the > >> partition holds default values. > >> > >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT; > > > > Yes, that could be done. But I don't think it's correct to say that > > the partition holds default values. Let's back up and ask what the > > word "default" means. The relevant definition (according to Google or > > whoever they stole it from) is: > > > > a preselected option adopted by a computer program or other mechanism > > when no alternative is specified by the user or programmer. > > > > So, a default *value* is the value that is used when no alternative is > > specified by the user or programmer. We have that concept, but it's > > not what we're talking about here: that's configured by applying the > > DEFAULT property to a column. Here, we're talking about the default > > *partition*, or in other words the *partition* that is used when no > > alternative is specified by the user or programmer. So, that's why I > > proposed the syntax I did. The partition doesn't contain default > > values; it is itself a default. > > Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more > natural. > > > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Ashutosh, On Tue, Apr 11, 2017 at 6:02 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe > wrote: > > Hi Rahila, > > > > > > With your latest patch: > > > > Consider a case when a table is partitioned on a boolean key. > > > > Even when there are existing separate partitions for 'true' and > > > > 'false', still default partition can be created. > > > > > > I think this should not be allowed. > > Well, boolean columns can have "NULL" values which will go into > default partition if no NULL partition exists. So, probably we should > add check for NULL partition there. I have checked for NULLs too, and the default partition can be created even when there are partitions for each TRUE, FALSE and NULL. Consider the example below: postgres=# CREATE TABLE list_partitioned ( a bool ) PARTITION BY LIST (a); CREATE TABLE postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN ('false'); CREATE TABLE postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN ('true'); CREATE TABLE postgres=# CREATE TABLE part_3 PARTITION OF list_partitioned FOR VALUES IN (null); CREATE TABLE postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR VALUES IN (DEFAULT); CREATE TABLE Regards, Jeevan Ladhe
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, I tried to review the code, and here are some of my early comments: 1. When I configure using "-Werror", I see unused variable in function DefineRelation: tablecmds.c: In function ‘DefineRelation’: tablecmds.c:761:17: error: unused variable ‘partdesc’ [-Werror=unused-variable] PartitionDesc partdesc; ^ 2. Typo in comment: + /* + * When adding a list partition after default partition, scan the + * default partiton for rows satisfying the new partition + * constraint. If found dont allow addition of a new partition. + * Otherwise continue with the creation of new partition. + */ partition don't 3. I think instead of a period '.', it will be good if you can use semicolon ';' in following declaration similar to the comment for 'null_index'. + int def_index; /* Index of the default list partition. -1 for + * range partitioned tables */ 4. You may want to consider 80 column alignment for changes done in function get_qual_from_partbound, and other places as applicable. 5. It would be good if the patch has some test coverage that explains what is being achieved, what kind of error handling is done etc. 6. There are some places having code like following: + Node *value = lfirst(c); Const *val = lfirst(c); PartitionListValue *list_value = NULL; + if (IsA(value, DefElem)) The additional variable is not needed and you can call IsA on val itself. 7. Also, in places like below where you are just trying to check for node is a DefaultElem, you can avoid an extra variable: + foreach(cell1, bspec->listdatums) + { + Node *value = lfirst(cell1); + if (IsA(value, DefElem)) + { + def_elem = true; + *defid = inhrelid; + } Can be written as: + foreach(cell1, bspec->listdatums) + { + if (IsA(lfirst(cell1), DefElem)) + { + def_elem = true; + *defid = inhrelid; + } + } Regards, Jeevan Ladhe On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe wrote: > Hi Rahila, > > > With your latest patch: > > Consider a case when a table is partitioned on a boolean key. > > Even when there are existing separate partitions for 'true' and > > 'false', still default partition can be created. > > > I think this should not be allowed. > > > Consider following case: > > > postgres=# CREATE TABLE list_partitioned ( > > a bool > > ) PARTITION BY LIST (a); > > CREATE TABLE > > postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN > ('false'); > > CREATE TABLE > > postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN > ('true'); > > CREATE TABLE > > postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR > VALUES IN (DEFAULT); > > CREATE TABLE > > > The creation of table part_default should have failed instead. > > > Thanks, > > Jeevan Ladhe > > > > On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske wrote: > >> >> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed >> wrote: >> >>> Hello, >>> >>> Thanks a lot for testing and reporting this. Please find attached an >>> updated patch with the fix. The patch also contains a fix >>> regarding operator used at the time of creating expression as default >>> partition constraint. This was notified offlist by Amit Langote. >>> >>> Thank you, >>> Rahila Syed >>> >>> >> Could probably use some more extensive testing, but all examples I had on >> my previously mentioned blog post are now working. >> >> Keith >> >> >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, With your latest patch: Consider a case when a table is partitioned on a boolean key. Even when there are existing separate partitions for 'true' and 'false', still default partition can be created. I think this should not be allowed. Consider following case: postgres=# CREATE TABLE list_partitioned ( a bool ) PARTITION BY LIST (a); CREATE TABLE postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN ('false'); CREATE TABLE postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN ('true'); CREATE TABLE postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR VALUES IN (DEFAULT); CREATE TABLE The creation of table part_default should have failed instead. Thanks, Jeevan Ladhe On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske wrote: > > On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed > wrote: > >> Hello, >> >> Thanks a lot for testing and reporting this. Please find attached an >> updated patch with the fix. The patch also contains a fix >> regarding operator used at the time of creating expression as default >> partition constraint. This was notified offlist by Amit Langote. >> >> Thank you, >> Rahila Syed >> >> > Could probably use some more extensive testing, but all examples I had on > my previously mentioned blog post are now working. > > Keith > >
Re: [HACKERS] Adding support for Default partition in partitioning
Hi Rahila, IIUC, your default_partition_v3.patch is trying to implement an error if new partition is added to a table already having a default partition. I too tried to run the test and similar to Rushabh, I see the server is crashing with the given test. However, if I reverse the order of creating the partitions, i.e. if I create a partition with list first and later create the default partition. The reason is, while defining new relation DefineRelation() checks for overlapping partitions by calling check_new_partition_bound(). Where in case of list partitions it assumes that the ndatums should be > 0, but in case of default partition that is 0. The crash here seems to be coming because, following assertion getting failed in function check_new_partition_bound(): Assert(boundinfo && boundinfo->strategy == PARTITION_STRATEGY_LIST && (boundinfo->ndatums > 0 || boundinfo->has_null)); So, I think the error you have added needs to be moved before this assertion: @@ -690,6 +715,12 @@ check_new_partition_bound(char *relname, Relation parent, Node *bound) boundinfo->strategy == PARTITION_STRATEGY_LIST && (boundinfo->ndatums > 0 || boundinfo->has_null)); + if (boundinfo->has_def) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("parent table \"%s\" has a default partition", + RelationGetRelationName(parent; If I do so, the server does not run into crash, and instead throws an error: postgres=# CREATE TABLE list_partitioned ( a int ) PARTITION BY LIST (a); CREATE TABLE postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR VALUES IN (DEFAULT); CREATE TABLE postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN (4,5); ERROR: parent table "list_partitioned" has a default partition Regards, Jeevan Ladhe On Mon, Mar 27, 2017 at 12:10 PM, Rushabh Lathia wrote: > I applied the patch and was trying to perform some testing, but its > ending up with server crash with the test shared by you in your starting > mail: > > postgres=# CREATE TABLE list_partitioned ( > postgres(# a int > postgres(# ) PARTITION BY LIST (a); > CREATE TABLE > postgres=# > postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR > VALUES IN (DEFAULT); > CREATE TABLE > > postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN > (4,5); > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > > Apart from this, few more explanation in the patch is needed to explain the > changes for the DEFAULT partition. Like I am not quite sure what exactly > the > latest version of patch supports, like does that support the tuple row > movement, > or adding new partition will be allowed having partition table having > DEFAULT > partition, which is quite difficult to understand through the code changes. > > Another part which is missing in the patch is the test coverage, adding > proper test coverage, which explain what is supported and what's not. > > Thanks, > > On Fri, Mar 24, 2017 at 3:25 PM, Rahila Syed > wrote: > >> Hello Rushabh, >> >> Thank you for reviewing. >> Have addressed all your comments in the attached patch. The attached >> patch currently throws an >> error if a new partition is added after default partition. >> >> >Rather then adding check for default here, I think this should be >> handle inside >> >get_qual_for_list(). >> Have moved the check inside get_qual_for_partbound() as needed to do some >> operations >> before calling get_qual_for_list() for default partitions. >> >> Thank you, >> Rahila Syed >> >> On Tue, Mar 21, 2017 at 11:36 AM, Rushabh Lathia < >> rushabh.lat...@gmail.com> wrote: >> >>> I picked this for review and noticed that patch is not getting >>> cleanly complied on my environment. >>> >>> partition.c: In function ‘RelationBuildPartitionDesc’: >>> partition.c:269:6: error: ISO C90 forbids mixed declarations and code >>> [-Werror=declaration-after-statement] >>> Const*val = lfirst(c); >>> ^ >>> partition.c: In function ‘generate_partition_qual’: >>> partition.c:1590:2: error: ISO C90 forbids mixed declarations and code >>> [-Werror=declaration-after-statement] >>> PartitionBoundSpec *spec = (PartitionBoundSpec *)bound; >>> ^ >>> partition.c: In function ‘get_partition_for_tuple’: >>> partition.c:1820:5: error: array subscript has type ‘char’ >>> [-Werr
Re: [HACKERS] Bug in get_partition_for_tuple
Hi Amit, I was able to reproduce the crash, and with the attached patch the crash goes away. Also, "make check-world" passes clean. Patch looks good to me. However, In following comment in your test: -- check routing error through a list partitioned table when they key is null I think you want to say: -- check routing error through a list partitioned table when the key is null Thanks, Jeevan Ladhe On Fri, Mar 10, 2017 at 8:26 AM, Amit Langote wrote: > Just observed a crash due to thinko in the logic that handles NULL > partition key. Absence of null-accepting partition in this case should > have caused an error, instead the current code proceeds with comparison > resulting in crash. > > create table p (a int, b char) partition by list (b); > create table p1 partition of p for values in ('a'); > insert into p values (1); -- crashes > > Attached patch fixes that and adds a test. > > Thanks, > Amit > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
Re: [HACKERS] pgbench more operators & functions
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:tested, passed The patch looks good to me now. Passing this to committer. The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench more operators & functions
Hi, The patch has correct precedence now. Further minor comments: 1. About documentation, I think it will be good idea to arrange the operators table with the precedence and add a line at top: "In decreasing order of precedence". 2. You may want to remove the comment: + /* should it do a lazy evaluation of the branch? */ Regards, Jeevan Ladhe
Re: [HACKERS] pgbench more operators & functions
On Sat, Jul 9, 2016 at 12:14 PM, Fabien COELHO wrote: > >> Here is a simple patch which adds a bunch of operators (bitwise: & | ^ ~, >> comparisons: =/== <>/!= < <= > >=, logical: and/&& or/|| xor/^^ not/!) and >> functions (exp ln if) to pgbench. I've tried to be pg's SQL compatible where >> appropriate. >> >> Also attached is a simple test script. > > > Here is a sightly updated version. > Hi Fabien, I did the review of your patch and here are my views on your patch. Purpose of the patch: = This patch introduces extensive list of new operators and functions that can be used in expressions in pgbench transaction scripts(given with -f option). Here is the list of operators and functions introduced by this patch: New operators: -- bitwise: <<, >>, &, |, ^/#, ~ comparisons: =/==, <>/!=, <, <=, >, >= logical: and/&&, or/||, xor/^^, not, ! New functions: -- exp, ln, if I see there had been a discussion about timing for submission of patch, but not much about what the patch is doing, so I believe the purpose of patch is quite acceptable. Currently there are limited number of operators available in pgbench. So, I think these new operators definitely make a value addition towards custom scripts. Documentation: = I could build the documentation without any errors. New operators and functions are well categorized and added in proper sections of existing pgbench documentation. I have a small suggestion here: Earlier we had only few number of operators, so it was OK to have the operators embedded in the description of '\set' command itself, but now as we have much more number of operators will it be a good idea to have a table of operators similar to that of functions. We need not have several columns here like description, example etc., but a short table just categorizing the operators would be sufficient. Initial Run: I was able to apply patch with 'patch -p1'. The testcase file(functions.sql) given along the patch gives an expected output. Further testing and review: === 1. Postgres treats '^' as exponentiation rather than XOR, and '#' as XOR. Personally, I think it can cause confusion, so it will be better if we can stick to the behavior of Postgres mathematical operators. 2. I could not see any tests for bitwise operators in the functions.sql file that you have attached. 3. Precedence: a. Unary operators have more precedence than binary operators. So, UNOT and UINV should have precedence next to UMINUS. I tried couple of test expressions using C Vs your patch(pgbench) expression result_in_C result_in_pgbench (~14-14+2) -27 -3 (!14-14+2) -12 0 b. Similarly shift operators should take more precedence over other bitwise operators: expression result_in_C result_in_pgbench (4|1<<1) 6 10 (4^5&3) 5 1 c. Also, comparison would take more precedence over bitwise operators(&,|,^) but shift operators. expression result_in_C result_in_pgbench (2&1<3) 1 0 In backend/parser/gram.y, I see that unary operators are given higher precedence than other operators, but it too does not have explicit precedence defined for bitwise operators. I tried to check Postgres documentation for operator precedence, but in 'Lexical Structure'[1] the documentation does not mention anything about bitwise operators. Also, SQL standards 99 does not talk much about operator precedence. I may be wrong while trying to understand the precedence you defined here and you might have picked this per some standard, but do you have any reference which you considered for this? 4. If we are going to stick to current precedence, I think it will be good idea to document it. 5. Sorry, I was not able to understand the "should exist" comment in following snippet. +"xor" { return XOR_OP; } /* should exist */ +"^^" { return XOR_OP; } /* should exist */ 7. You may want to reword following comment: + else /* cannot get there */ To + else /* cannot get here */ 8. + case PGBENCH_IF: + /* should it do a lazy evaluation of the branch? */ + Assert(nargs == 3); + *retval = coerceToBool(&vargs[0]) ? vargs[1] : vargs[2]; I believe ternary operator does the lazy evaluation internally, but to be sure you may consider rewriting this as following: if (coerceToBool(&vargs[0])) *retval = vargs[1]; else *retval = vargs[2]; Conclusion: === I have tested the patch and each of the operator is implemented correctly. The only concern I have is precedence, otherwise the patch seems to be doing what it is supposed to do. [1]https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html Regards, Jeevan Ladhe. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers