On Thu, Jan 25, 2018 at 10:39 AM, Robert Haas wrote:
> On Mon, Jan 22, 2018 at 9:46 AM, Tom Lane wrote:
>> Robert Haas writes:
>>> Tom, do you want to double-check that this fixes it for you?
>>
>> I can confirm that a valgrind run succeeded for me with the patch
>> in place.
>
> Committed. Sor
On Mon, Jan 22, 2018 at 9:46 AM, Tom Lane wrote:
> Robert Haas writes:
>> Tom, do you want to double-check that this fixes it for you?
>
> I can confirm that a valgrind run succeeded for me with the patch
> in place.
Committed. Sorry for the delay.
--
Robert Haas
EnterpriseDB: http://www.ente
Robert Haas writes:
> Tom, do you want to double-check that this fixes it for you?
I can confirm that a valgrind run succeeded for me with the patch
in place.
regards, tom lane
On Mon, Jan 22, 2018 at 2:44 AM, Amit Khandekar wrote:
> Yes, right, that's what is happening. It is not happening on an Assert
> though (there is no assert in that function). It is happening when we
> try to access the array here :
>
> if (proute->subplan_partition_offsets &&
>
On 22 January 2018 at 02:40, Robert Haas wrote:
> On Sun, Jan 21, 2018 at 1:43 AM, Tom Lane wrote:
>> Robert Haas writes:
>>> Committed with a bunch of mostly-cosmetic revisions.
>>
>> Buildfarm member skink has been unhappy since this patch went in.
>> Running the regression tests under valgrin
On Sun, Jan 21, 2018 at 1:43 AM, Tom Lane wrote:
> Robert Haas writes:
>> Committed with a bunch of mostly-cosmetic revisions.
>
> Buildfarm member skink has been unhappy since this patch went in.
> Running the regression tests under valgrind easily reproduces the
> failure. Now, I might be wron
Robert Haas writes:
> Committed with a bunch of mostly-cosmetic revisions.
Buildfarm member skink has been unhappy since this patch went in.
Running the regression tests under valgrind easily reproduces the
failure. Now, I might be wrong about which of the patches committed
on Friday caused the
On Fri, Jan 19, 2018 at 4:37 AM, Amit Khandekar wrote:
> Attached rebased patch.
Committed with a bunch of mostly-cosmetic revisions. I removed the
macro you added, which has a multiple evaluation hazard, and just put
that logic back into the function. I don't think it's likely to
matter for pe
On 16 January 2018 at 09:17, David Rowley wrote:
> On 16 January 2018 at 01:09, Robert Haas wrote:
>> On Sun, Jan 14, 2018 at 6:57 AM, Amit Khandekar
>> wrote:
>>> Even where partitions are present, in the usual case where there are
>>> Instead of a bool array, we can even make it a Bitmapset.
On 16 January 2018 at 01:09, Robert Haas wrote:
> On Sun, Jan 14, 2018 at 6:57 AM, Amit Khandekar
> wrote:
>> Even where partitions are present, in the usual case where there are
>> Instead of a bool array, we can even make it a Bitmapset. But I think
>> access would become slower as compared to
On Sun, Jan 14, 2018 at 6:57 AM, Amit Khandekar wrote:
> Even where partitions are present, in the usual case where there are
> no transition tables we won't require per-leaf map at all [1]. So I
> think we should keep mt_per_sub_plan_maps only for the case where
> per-leaf map is not allocated. A
On 14 January 2018 at 17:27, Amit Khandekar wrote:
> On 13 January 2018 at 02:56, Robert Haas wrote:
> > I guess I'm inclined to keep mt_per_sub_plan_maps for the case where
> > there are no partitions, but not use it when partitions are present.
> > What do you think about that?
>
> Even where p
On 13 January 2018 at 02:56, Robert Haas wrote:
> On Fri, Jan 12, 2018 at 12:23 PM, Amit Khandekar
> wrote:
>>> (1) if they need it by subplan index, first use
>>> subplan_partition_offsets to convert it to a per-leaf index
>>
>> Before that, we need to check if there *is* an offset array. If th
On Fri, Jan 12, 2018 at 12:23 PM, Amit Khandekar wrote:
>> (1) if they need it by subplan index, first use
>> subplan_partition_offsets to convert it to a per-leaf index
>
> Before that, we need to check if there *is* an offset array. If there
> are no partitions, there is only going to be a per-s
On 12 January 2018 at 20:24, Robert Haas wrote:
> On Fri, Jan 12, 2018 at 5:12 AM, Amit Khandekar
> wrote:
>> The reason why I am having map_required field inside a structure along
>> with the map, as against a separate array, is so that we can do the
>> on-demand allocation for both per-leaf ar
On Fri, Jan 12, 2018 at 5:12 AM, Amit Khandekar wrote:
> The reason why I am having map_required field inside a structure along
> with the map, as against a separate array, is so that we can do the
> on-demand allocation for both per-leaf array and per-subplan array.
Putting the map_required fiel
On 12 January 2018 at 01:18, Robert Haas wrote:
> On Thu, Jan 11, 2018 at 6:07 AM, Amit Khandekar
> wrote:
>> In the first paragraph of my explanation, I was explaining why two
>> Transition capture states does not look like a good idea to me :
>
> Oh, sorry. I didn't read what you wrote carefu
On Thu, Jan 11, 2018 at 6:07 AM, Amit Khandekar wrote:
> In the first paragraph of my explanation, I was explaining why two
> Transition capture states does not look like a good idea to me :
Oh, sorry. I didn't read what you wrote carefully enough, I guess.
I see your points. I think that ther
On 9 January 2018 at 23:07, Robert Haas wrote:
> On Thu, Jan 4, 2018 at 1:18 AM, Amit Khandekar wrote:
>> --
>> 1. ExecUpdate() needs to revert back tcs_map value changed by ExecInsert()
>> --
>>
It seems like the ON CONFLICT stuff handled that by adding a
>>>
On 11 January 2018 at 10:44, David Rowley wrote:
>>> 18. Why two RESET SESSION AUTHORIZATIONs?
>>>
>>> reset session authorization;
>>> drop trigger trig_d_1_15 ON part_d_1_15;
>>> drop function func_d_1_15();
>>> -- Policy expression contains SubPlan
>>> reset session authorization;
>>
>> The sec
Thanks for making those changes.
On 11 January 2018 at 04:00, Amit Khandekar wrote:
> Yes, I understand that there won't be any update scan plans. But, with
> the modifications done in ExecInitModifyTable(), I wanted to run that
> code with this scenario where there are no partitions, to make sur
On Fri, Jan 5, 2018 at 3:25 PM, Robert Haas wrote:
> On Fri, Jan 5, 2018 at 7:12 AM, Amit Khandekar wrote:
>> The above patch is to be applied over the last remaining preparatory
>> patch, now named (and attached) :
>> 0001-Refactor-CheckConstraint-related-code.patch
>
> Committed that one, too.
On Thu, Jan 4, 2018 at 1:18 AM, Amit Khandekar wrote:
> --
> 1. ExecUpdate() needs to revert back tcs_map value changed by ExecInsert()
> --
>
>>> It seems like the ON CONFLICT stuff handled that by adding a
>>> second TransitionCaptureState pointer to ModifyTable,
On 4 January 2018 at 02:52, David Rowley wrote:
> I'll try to look at the tests tomorrow and also do some testing.
I've made a pass over the tests. Again, sometimes I'm probably a bit
pedantic. The reason for that is that the tests are not that easy to
follow. Moving creation and cleanup of objec
On Fri, Jan 5, 2018 at 7:12 AM, Amit Khandekar wrote:
> The above patch is to be applied over the last remaining preparatory
> patch, now named (and attached) :
> 0001-Refactor-CheckConstraint-related-code.patch
Committed that one, too.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
T
On Wed, Jan 3, 2018 at 6:29 AM, Amit Khandekar wrote:
> ExecSetupPartitionTupleRouting() now returns PartitionTupleRouting *.
>
> Did this change in v3 version of
> 0001-Encapsulate-partition-related-info-in-a-structure.patch
I'll have to come back to some of the other open issues, but 0001 and
0
On 4 January 2018 at 02:52, David Rowley wrote:
> I'll try to look at the tests tomorrow and also do some testing. So
> far I've only read the code and the docs.
There are a few more things I noticed on another pass I made today:
20. "carried" -> "carried out the"
+ would have identified
Robert, for tracking purpose, below I have consolidated your review
items on which we are yet to conclude. Let me know if you have more
comments on the points which I made.
--
1. ExecUpdate() needs to revert back tcs_map value changed by ExecInsert()
--
>> + /*
>>
> On 3 January 2018 at 11:42, Amit Khandekar wrote:
>> [...] So it make perfect sense for
>> ExecSetupPartitionTupleRouting() to palloc and return a pointer. Sorry
>> for the noise. Will share the change in an upcoming patch version.
>> Thanks !
>
> ExecSetupPartitionTupleRouting() now returns Par
On 20 December 2017 at 11:52, Amit Khandekar wrote:
> On 14 December 2017 at 08:11, Amit Langote
> wrote:
>>
>> Regarding ExecSetupChildParentMap(), it seems to me that it could simply
>> be declared as
>>
>> static void ExecSetupChildParentMap(ModifyTableState *mtstate);
>>
>> Looking at the pl
On 1 January 2018 at 21:43, Amit Khandekar wrote:
> On 16 December 2017 at 03:09, Robert Haas wrote:
>> + /*
>> + * UPDATEs set the transition capture map only when a new subplan
>> + * is chosen. But for INSERTs, it is set for each row. So after
>> + * INSERT, we need to revert back to the map
On 2 January 2018 at 10:56, David Rowley wrote:
> On 23 December 2017 at 04:00, Amit Khandekar wrote:
>> On 15 December 2017 at 18:28, Robert Haas wrote:
>>> -PartitionDispatch **pd,
>>> -ResultRelInfo ***partitions,
>>> -TupleConversionMap ***tup_conv_maps,
>>> -TupleTableSlot *
On 16 December 2017 at 03:09, Robert Haas wrote:
>
> - map = ptr->partition_tupconv_maps[leaf_part_index];
> + map = ptr->parentchild_tupconv_maps[leaf_part_index];
>
> I don't think there's any reason to rename this. In previous patch
> versions, you had multiple arrays of tuple conversion maps
On 23 December 2017 at 04:00, Amit Khandekar wrote:
> On 15 December 2017 at 18:28, Robert Haas wrote:
>> -PartitionDispatch **pd,
>> -ResultRelInfo ***partitions,
>> -TupleConversionMap ***tup_conv_maps,
>> -TupleTableSlot **partition_tuple_slot,
>> -int *num_parted, int *num
On 16 December 2017 at 03:09, Robert Haas wrote:
> started another review pass over the main patch, so here are
> some comments about that.
I am yet to address all the comments, but meanwhile, below are some
specific points ...
> + if (!partrel)
> + {
> + /*
> + * We locked all the partitions a
On Fri, Dec 15, 2017 at 7:58 AM, Robert Haas wrote:
> Reviewing the preparatory patch:
I started another review pass over the main patch, so here are some
comments about that. This is unfortunately not a complete review,
however.
- map = ptr->partition_tupconv_maps[leaf_part_index];
+ map = ptr
On Wed, Dec 13, 2017 at 5:18 AM, Amit Khandekar wrote:
> Amit Langote informed me off-list, - along with suggestions for
> changes - that my patch needs a rebase. Attached is the rebased
> version. I have also bumped the patch version number (now v29),
> because this as additional changes, again,
Thanks for the updated patches, Amit.
Some review comments.
Forgot to remove the description of update_rri and num_update_rri in the
header comment of ExecSetupPartitionTupleRouting().
-
+extern void pull_child_partition_columns(Relation rel,
+ Relation parent,
+
While addressing Thomas's point about test scenarios not yet covered,
I observed the following ...
Suppose an UPDATE RLS policy with a WITH CHECK clause is defined on
the target table. Now In ExecUpdate(), the corresponding WCO qual gets
executed *before* the partition constraint check, as per exi
On Mon, Nov 27, 2017 at 5:28 PM, Amit Khandekar wrote:
> So, in the upcoming patch version, I am intending to include the above
> two, and if possible, Robert's idea of re-using is_partition_attr()
> for pull_child_partition_columns().
Discussions are still going on, so moved to next CF.
--
Mich
On 24 November 2017 at 10:52, Amit Langote
wrote:
> On 2017/11/23 21:57, Amit Khandekar wrote:
>> If we collect the partition keys in expand_partitioned_rtentry(), we
>> need to pass the root relation also, so that we can convert the
>> partition key attributes to root rel descriptor. And the othe
On 2017/11/23 21:57, Amit Khandekar wrote:
> If we collect the partition keys in expand_partitioned_rtentry(), we
> need to pass the root relation also, so that we can convert the
> partition key attributes to root rel descriptor. And the other thing
> is, may be, we can check beforehand (in expand
On 7 November 2017 at 00:33, Robert Haas wrote:
> + /* The caller must have already locked all the partitioned tables. */
> + root_rel = heap_open(root_relid, NoLock);
> + *all_part_cols = NULL;
> + foreach(lc, partitioned_rels)
> + {
> + Index
On 21 November 2017 at 17:24, Amit Khandekar wrote:
> On 13 November 2017 at 18:25, David Rowley
> wrote:
>>
>> 30. The following chunk of code is giving me a headache trying to
>> verify which arrays are which size:
>>
>> ExecSetupPartitionTupleRouting(rel,
>>mtstate->resultRelInfo,
>>(
Thanks Amit.
Looking at the latest v25 patch.
On 2017/11/16 23:50, Amit Khandekar wrote:
> Below has the responses for both Amit's and David's comments, starting
> with Amit's
> On 2 November 2017 at 12:40, Amit Langote
> wrote:
>> On 2017/10/24 0:15, Amit Khandekar wrote:
>>> On 16 Octobe
David Rowley wrote:
> 5. Triggers. Do we need a new "TG_" tag to allow trigger functions to
> do something special when the DELETE/INSERT is a partition move? I
> have audit tables in mind here it may appear as though a user
> performed a DELETE when they actually performed an UPDATE giving
> visi
On 14 November 2017 at 01:55, David Rowley wrote:
> I'll try to continue with the review tomorrow, but I think some other
> reviews are also looming too.
I started looking at this again today. Here's the remainder of my review.
29. ExecSetupChildParentMap gets called here for non-partitioned rel
On Fri, Nov 10, 2017 at 4:42 PM, Amit Khandekar wrote:
> Attached is v23 patch that has just the above changes (and also
> rebased on hash-partitioning changes, like update.sql). I am still
> doing some sanity testing on this, although regression passes.
The test coverage[1] is 96.62%. Nice work
48 matches
Mail list logo
