On Mon, Apr 11, 2022 at 4:47 PM Amit Langote wrote:
> This one has been marked Returned with Feedback in the CF app, which
> makes sense given the discussion on -committers [1].
>
> Agree with the feedback given that it would be better to address *all*
> RI trigger check/action functions in the
On Thu, Apr 7, 2022 at 10:05 AM Amit Langote wrote:
> There were rebase conflicts with the recently committed
> execPartition.c/h changes. While fixing them, I thought maybe
> find_leaf_part_for_key() doesn't quite match in style with its
> neighbors in execPartition.h, so changed it to
>
There were rebase conflicts with the recently committed
execPartition.c/h changes. While fixing them, I thought maybe
find_leaf_part_for_key() doesn't quite match in style with its
neighbors in execPartition.h, so changed it to
ExecGetLeafPartitionForKey().
--
Amit Langote
EDB:
On Mon, Mar 14, 2022 at 6:28 PM Zhihong Yu wrote:
> On Mon, Mar 14, 2022 at 1:33 AM Amit Langote wrote:
>> On Tue, Jan 18, 2022 at 3:30 PM Amit Langote wrote:
>> > v13 is attached.
>>
>> I noticed that the recent 641f3dffcdf's changes to
>> get_constraint_index() made it basically unusable for
On Mon, Mar 14, 2022 at 1:33 AM Amit Langote
wrote:
> On Tue, Jan 18, 2022 at 3:30 PM Amit Langote
> wrote:
> > v13 is attached.
>
> I noticed that the recent 641f3dffcdf's changes to
> get_constraint_index() made it basically unusable for this patch's
> purposes.
>
> Reading in the thread that
On Tue, Jan 18, 2022 at 3:30 PM Amit Langote wrote:
> v13 is attached.
I noticed that the recent 641f3dffcdf's changes to
get_constraint_index() made it basically unusable for this patch's
purposes.
Reading in the thread that led to 641f3dffcdf why
get_constraint_index() was changed the way it
Thanks for the review.
On Tue, Dec 21, 2021 at 5:54 PM Zhihong Yu wrote:
> Hi,
>
> + int lockflags = 0;
> + TM_Result test;
> +
> + lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS;
>
> The above assignment can be meged with the line where variable lockflags is
> declared.
On Mon, Dec 20, 2021 at 5:17 AM Amit Langote
wrote:
> On Mon, Dec 20, 2021 at 6:19 PM Zhihong Yu wrote:
> > On Sun, Dec 19, 2021 at 10:20 PM Amit Langote
> wrote:
> >>
> >> On Mon, Dec 20, 2021 at 2:00 PM Corey Huinker
> wrote:
> >> > Sorry for the delay. This patch no longer applies, it has
>
>
>
> Good catch, thanks. Patch updated.
>
>
>
Applies clean. Passes check-world.
On Mon, Dec 20, 2021 at 6:19 PM Zhihong Yu wrote:
> On Sun, Dec 19, 2021 at 10:20 PM Amit Langote wrote:
>>
>> On Mon, Dec 20, 2021 at 2:00 PM Corey Huinker
>> wrote:
>> > Sorry for the delay. This patch no longer applies, it has some conflict
>> > with
On Sun, Dec 19, 2021 at 10:20 PM Amit Langote
wrote:
> On Mon, Dec 20, 2021 at 2:00 PM Corey Huinker
> wrote:
> > Sorry for the delay. This patch no longer applies, it has some conflict
> with d6f96ed94e73052f99a2e545ed17a8b2fdc1fb8a
>
> Thanks Corey for the heads up. Rebased with some
On Mon, Dec 20, 2021 at 2:00 PM Corey Huinker wrote:
> Sorry for the delay. This patch no longer applies, it has some conflict with
> d6f96ed94e73052f99a2e545ed17a8b2fdc1fb8a
Thanks Corey for the heads up. Rebased with some cosmetic adjustments.
--
Amit Langote
EDB:
>
>
>
> I wasn't able to make much inroads into how we might be able to get
> rid of the DETACH-related partition descriptor hacks, the item (3),
> though I made some progress on items (1) and (2).
>
> For (1), the attached 0001 patch adds a new isolation suite
> fk-snapshot.spec to exercise
On Sat, Nov 13, 2021 at 5:43 AM Tom Lane wrote:
> Amit Langote writes:
> > On Fri, Nov 12, 2021 at 8:19 AM Tom Lane wrote:
> >> Anyway, I think that (1) we should write some more test cases around
> >> this behavior, (2) you need to establish the snapshot to use in two
> >> different ways for
Amit Langote writes:
> On Fri, Nov 12, 2021 at 8:19 AM Tom Lane wrote:
>> Anyway, I think that (1) we should write some more test cases around
>> this behavior, (2) you need to establish the snapshot to use in two
>> different ways for the RI_FKey_check and ri_Check_Pk_Match cases,
>> and (3)
Amit Langote writes:
> Whatever mechanism we will use would still need to involve setting a
> global Snapshot variable though, right?
In v14 we'll certainly still be passing the snapshot(s) to SPI, which will
eventually make the snapshot active. With your patch, since we're just
handing the
On Fri, Nov 12, 2021 at 10:58 AM Tom Lane wrote:
> I wrote:
> > Anyway, I think that (1) we should write some more test cases around
> > this behavior, (2) you need to establish the snapshot to use in two
> > different ways for the RI_FKey_check and ri_Check_Pk_Match cases,
> > and (3)
On Fri, Nov 12, 2021 at 8:19 AM Tom Lane wrote:
> Amit Langote writes:
> > Rebased patches attached.
>
> I've spent some more time digging into the snapshot-management angle.
Thanks for looking at this.
> I think you are right that the crosscheck_snapshot isn't really an
> issue because the
I wrote:
> Anyway, I think that (1) we should write some more test cases around
> this behavior, (2) you need to establish the snapshot to use in two
> different ways for the RI_FKey_check and ri_Check_Pk_Match cases,
> and (3) something's going to have to be done to repair the behavior
> in v14
Alvaro Herrera writes:
> I think we (I) should definitely pursue fixing whatever was broken by
> DETACH CONCURRENTLY, back to pg14, independently of this patch ... but
> I would appreciate some insight into what the problem is.
Here's what I'm on about:
regression=# create table pk (f1 int
On 2021-Nov-11, Tom Lane wrote:
> It appears to me that commit 71f4c8c6f (DETACH PARTITION CONCURRENTLY)
> broke the semantics here, because now things work differently with a
> partitioned PK table than with a plain table, thanks to not bothering
> to distinguish questions of how to handle
Amit Langote writes:
> Rebased patches attached.
I've spent some more time digging into the snapshot-management angle.
I think you are right that the crosscheck_snapshot isn't really an
issue because the executor pays no attention to it for SELECT, but
that doesn't mean that there's no problem,
>
> Rebased patches attached.
I'm reviewing the changes since v6, which was my last review.
Making ExecLockTableTuple() it's own function makes sense.
Snapshots are now accounted for.
The changes that account for n-level partitioning makes sense as well.
Passes make check-world.
Not user
On Tue, Jul 6, 2021 at 1:56 AM vignesh C wrote:
> The 2nd patch does not apply on Head, please post a rebased version:
> error: patch failed: src/backend/utils/adt/ri_triggers.c:337
> error: src/backend/utils/adt/ri_triggers.c: patch does not apply
Thanks for the heads up.
Rebased patches
On Sun, Apr 4, 2021 at 1:51 PM Amit Langote wrote:
>
> On Fri, Apr 2, 2021 at 11:55 PM Zhihong Yu wrote:
> >
> > Hi,
> >
> > + skip = !ExecLockTableTuple(erm->relation, , markSlot,
> > + estate->es_snapshot,
> > estate->es_output_cid,
> > +
On Fri, Apr 2, 2021 at 11:55 PM Zhihong Yu wrote:
>
> Hi,
>
> + skip = !ExecLockTableTuple(erm->relation, , markSlot,
> + estate->es_snapshot, estate->es_output_cid,
> + lockmode, erm->waitPolicy, _needed);
> + if
Hi Alvaro,
On Sat, Apr 3, 2021 at 12:01 AM Alvaro Herrera wrote:
> On 2021-Apr-02, Amit Langote wrote:
>
> > On Sat, Mar 20, 2021 at 10:21 PM Amit Langote
> > wrote:
> > > Updated patches attached. Sorry about the delay.
> >
> > Rebased over the recent DETACH PARTITION CONCURRENTLY work.
> >
On 2021-Apr-02, Amit Langote wrote:
> On Sat, Mar 20, 2021 at 10:21 PM Amit Langote wrote:
> > Updated patches attached. Sorry about the delay.
>
> Rebased over the recent DETACH PARTITION CONCURRENTLY work.
> Apparently, ri_ReferencedKeyExists() was using the wrong snapshot.
Hmm, I wonder if
Hi,
+ skip = !ExecLockTableTuple(erm->relation, , markSlot,
+ estate->es_snapshot,
estate->es_output_cid,
+ lockmode, erm->waitPolicy, _needed);
+ if (skip)
It seems the variable skip is only used above. The variable
On Sat, Mar 20, 2021 at 10:21 PM Amit Langote wrote:
> Updated patches attached. Sorry about the delay.
Rebased over the recent DETACH PARTITION CONCURRENTLY work.
Apparently, ri_ReferencedKeyExists() was using the wrong snapshot.
--
Amit Langote
EDB: http://www.enterprisedb.com
On Mon, Mar 8, 2021 at 11:41 PM Amit Langote wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane wrote:
> > I guess I'm disturbed by the idea
> > that we'd totally replace the implementation technology for only one
> > variant of foreign key checks. That means that there'll be a lot
> > of minor
On Mon, Mar 8, 2021 at 11:41 PM Amit Langote wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane wrote:
> > Lastly, ri_PerformCheck is pretty careful about not only which
> > snapshot it uses, but which *pair* of snapshots it uses, because
> > sometimes it needs to worry about data changes since
On Thu, Mar 4, 2021 at 5:15 AM Tom Lane wrote:
> I took a quick look at this.
Thanks a lot for the review.
> I guess I'm disturbed by the idea
> that we'd totally replace the implementation technology for only one
> variant of foreign key checks. That means that there'll be a lot
> of minor
I took a quick look at this. I guess I'm disturbed by the idea
that we'd totally replace the implementation technology for only one
variant of foreign key checks. That means that there'll be a lot
of minor details that don't act the same depending on context. One
point I was just reminded of by
Hi amit,
(sorry about not cc the hacker list)
I have an issue about command id here.
It's probably not directly related to your patch, so I am sorry if it bothers
you.
+ /*
+* Start the scan. To make the changes of the current command visible to
+* the scan and for
On Mon, Mar 1, 2021 at 3:14 PM Corey Huinker wrote:
>> > It seems to me 1 (RI_PLAN_CHECK_LOOKUPPK) is still alive. (Yeah, I
>> > know that doesn't mean the usefulness of the macro but the mechanism
>> > the macro suggests, but it is confusing.) On the other hand,
>> >
>
> > It seems to me 1 (RI_PLAN_CHECK_LOOKUPPK) is still alive. (Yeah, I
> > know that doesn't mean the usefulness of the macro but the mechanism
> > the macro suggests, but it is confusing.) On the other hand,
> > RI_PLAN_CHECK_LOOKUPPK_FROM_PK and RI_PLAN_LAST_ON_PK seem to be no
> > longer
On Wed, Jan 27, 2021 at 5:32 PM Kyotaro Horiguchi
wrote:
> At Sun, 24 Jan 2021 20:51:39 +0900, Amit Langote
> wrote in
> > Here's v5.
>
> At Mon, 25 Jan 2021 18:19:56 +0900, Amit Langote
> wrote in
> > > Anybody else want to look this patch over before I mark it Ready For
> > > Committer?
>
At Sun, 24 Jan 2021 20:51:39 +0900, Amit Langote
wrote in
> Here's v5.
At Mon, 25 Jan 2021 18:19:56 +0900, Amit Langote
wrote in
> > Anybody else want to look this patch over before I mark it Ready For
> > Committer?
>
> Would be nice to have others look it over. Thanks.
This nice
Hi Amit-san,
Thanks for the answer!
> If you only tested insert/update on the referencing table, I would've
> expected to see nothing in the result of that query, because the patch
> eliminates all use of SPI in that case. I suspect the CachedPlan*
> memory contexts you are seeing belong to
Hi Amit-san,
+ case TM_Invisible:
+ elog(ERROR, "attempted to lock invisible tuple");
+ break;
+
+ case TM_SelfModified:
+ case TM_BeingModified:
+ case TM_WouldBlock:
+ elog(ERROR, "unexpected table_tuple_lock
Yamada-san,
On Wed, Jan 27, 2021 at 8:51 AM Tatsuro Yamada
wrote:
> On 2021/01/25 18:19, Amit Langote wrote:
> > On Mon, Jan 25, 2021 at 9:24 AM Corey Huinker
> > wrote:
> >> Anybody else want to look this patch over before I mark it Ready For
> >> Committer?
> >
> > Would be nice to have
Hi Amit-san,
On 2021/01/25 18:19, Amit Langote wrote:
On Mon, Jan 25, 2021 at 9:24 AM Corey Huinker wrote:
On Sun, Jan 24, 2021 at 6:51 AM Amit Langote wrote:
Here's v5.
v5 patches apply to master.
Suggested If/then optimization is implemented.
Suggested case merging is implemented.
On Mon, Jan 25, 2021 at 7:01 PM Amit Langote wrote:
> On Mon, Jan 25, 2021 at 6:06 PM Keisuke Kuroda
> wrote:
> > However, as already mentioned, the problem of memory bloat on DELETE
> > remains.
> > This can be solved by the patch in [1], but I think it is too much to apply
> > this patch only
Kuroda-san,
On Mon, Jan 25, 2021 at 6:06 PM Keisuke Kuroda
wrote:
> Hi, Amit-san,
>
> Nice patch. I have confirmed that this solves the problem in [1] with
> INSERT/UPDATE.
Thanks for testing.
> HEAD + patch
>name | bytes | pg_size_pretty
>
On Mon, Jan 25, 2021 at 9:24 AM Corey Huinker wrote:
> On Sun, Jan 24, 2021 at 6:51 AM Amit Langote wrote:
>> Here's v5.
>
> v5 patches apply to master.
> Suggested If/then optimization is implemented.
> Suggested case merging is implemented.
> Passes make check and make check-world yet again.
>
Hi, Amit-san,
Nice patch. I have confirmed that this solves the problem in [1] with
INSERT/UPDATE.
HEAD + patch
name | bytes | pg_size_pretty
--+---+
CachedPlanQuery | 10280 | 10 kB
CachedPlanSource | 14616 | 14 kB
CachedPlan | 13168 |
On Sun, Jan 24, 2021 at 6:51 AM Amit Langote
wrote:
> On Sun, Jan 24, 2021 at 11:26 AM Corey Huinker
> wrote:
> > On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu wrote:
> >>
> >> Hi,
>
> Thanks for the review.
>
> >> + for (i = 0; i < riinfo->nkeys; i++)
> >> + {
> >> + Oid
On Sun, Jan 24, 2021 at 11:26 AM Corey Huinker wrote:
> On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu wrote:
>>
>> Hi,
Thanks for the review.
>> + for (i = 0; i < riinfo->nkeys; i++)
>> + {
>> + Oid eq_opr = eq_oprs[i];
>> + Oid typeid = RIAttType(fk_rel,
On Sat, Jan 23, 2021 at 12:52 PM Zhihong Yu wrote:
> Hi,
>
> + for (i = 0; i < riinfo->nkeys; i++)
> + {
> + Oid eq_opr = eq_oprs[i];
> + Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]);
> + RI_CompareHashEntry *entry =
Hi,
+ for (i = 0; i < riinfo->nkeys; i++)
+ {
+ Oid eq_opr = eq_oprs[i];
+ Oid typeid = RIAttType(fk_rel, riinfo->fk_attnums[i]);
+ RI_CompareHashEntry *entry = ri_HashCompareOp(eq_opr, typeid);
+
+ if (pk_nulls[i] != 'n' &&
On Fri, Jan 22, 2021 at 3:22 PM Corey Huinker wrote:
>> I decided not to deviate from pk_ terminology so that the new code
>> doesn't look too different from the other code in the file. Although,
>> I guess we can at least call the main function
>> ri_ReferencedKeyExists() instead of
>
>
>
> I decided not to deviate from pk_ terminology so that the new code
> doesn't look too different from the other code in the file. Although,
> I guess we can at least call the main function
> ri_ReferencedKeyExists() instead of ri_PrimaryKeyExists(), so I've
> changed that.
>
I think
>
> I decided not to deviate from pk_ terminology so that the new code
> doesn't look too different from the other code in the file. Although,
> I guess we can at least call the main function
> ri_ReferencedKeyExists() instead of ri_PrimaryKeyExists(), so I've
> changed that.
>
I agree with
On Tue, Jan 19, 2021 at 12:00 PM Zhihong Yu wrote:
> + if (mapped_partkey_attnums[i] == pk_attnums[j])
> + {
> + partkey_vals[i] = pk_vals[j];
> + partkey_isnull[i] = pk_nulls[j] == 'n' ? true : false;
> + i++;
> +
On Tue, Jan 19, 2021 at 3:46 PM Corey Huinker wrote:
> v2 patch applies and passes make check and make check-world. Perhaps, given
> the missing break at line 418 without any tests failing, we could add another
> regression test if we're into 100% code path coverage. As it is, I think the
>
On Mon, Jan 18, 2021 at 9:45 PM Amit Langote
wrote:
> On Tue, Jan 19, 2021 at 2:47 AM Zhihong Yu wrote:
> >
> > Hi,
> > I was looking at this statement:
> >
> > insert into f select generate_series(1, 200, 2);
> >
> > Since certain generated values (the second half) are not in table p,
>
On Tue, Jan 19, 2021 at 2:56 PM Corey Huinker wrote:
>> In file included from
>> /home/japin/Codes/postgresql/Debug/../src/include/postgres.h:47:0,
>> from
>> /home/japin/Codes/postgresql/Debug/../src/backend/utils/adt/ri_triggers.c:24:
>>
>
>
> In file included from
> /home/japin/Codes/postgresql/Debug/../src/include/postgres.h:47:0,
> from
> /home/japin/Codes/postgresql/Debug/../src/backend/utils/adt/ri_triggers.c:24:
> /home/japin/Codes/postgresql/Debug/../src/backend/utils/adt/ri_triggers.c:
> In function
út 19. 1. 2021 v 3:08 odesílatel Amit Langote
napsal:
> On Tue, Jan 19, 2021 at 3:01 AM Pavel Stehule
> wrote:
> > po 18. 1. 2021 v 13:40 odesílatel Amit Langote
> napsal:
> >> I started with the check that's performed when inserting into or
> >> updating the referencing table to confirm that
On Tue, 19 Jan 2021 at 10:45, Amit Langote wrote:
> On Tue, Jan 19, 2021 at 2:47 AM Zhihong Yu wrote:
>>
>> Hi,
>> I was looking at this statement:
>>
>> insert into f select generate_series(1, 200, 2);
>>
>> Since certain generated values (the second half) are not in table p,
>> wouldn't
Thanks for the quick response.
+ if (mapped_partkey_attnums[i] == pk_attnums[j])
+ {
+ partkey_vals[i] = pk_vals[j];
+ partkey_isnull[i] = pk_nulls[j] == 'n' ? true : false;
+ i++;
+ break;
The
On Tue, Jan 19, 2021 at 2:47 AM Zhihong Yu wrote:
>
> Hi,
> I was looking at this statement:
>
> insert into f select generate_series(1, 200, 2);
>
> Since certain generated values (the second half) are not in table p, wouldn't
> insertion for those values fail ?
> I tried a scaled down
On Tue, Jan 19, 2021 at 3:01 AM Pavel Stehule wrote:
> po 18. 1. 2021 v 13:40 odesílatel Amit Langote
> napsal:
>> I started with the check that's performed when inserting into or
>> updating the referencing table to confirm that the new row points to a
>> valid row in the referenced relation.
po 18. 1. 2021 v 13:40 odesílatel Amit Langote
napsal:
> While discussing the topic of foreign key performance off-list with
> Robert and Corey (also came up briefly on the list recently [1], [2]),
> a few ideas were thrown around to simplify our current system of RI
> checks to enforce foreign
Hi,
I was looking at this statement:
insert into f select generate_series(1, 200, 2);
Since certain generated values (the second half) are not in table p,
wouldn't insertion for those values fail ?
I tried a scaled down version (1000th) of your example:
yugabyte=# insert into f select
While discussing the topic of foreign key performance off-list with
Robert and Corey (also came up briefly on the list recently [1], [2]),
a few ideas were thrown around to simplify our current system of RI
checks to enforce foreign keys with the aim of reducing some of its
overheads. The two
67 matches
Mail list logo
