On Sat, Oct 3, 2020 at 8:26 PM Amit Langote wrote:
> On Sat, Oct 3, 2020 at 8:15 PM Tomas Vondra
> wrote
> > On Sat, Oct 03, 2020 at 11:42:21AM +0900, Amit Langote wrote:
> > >On Fri, Oct 2, 2020 at 11:32 PM David G. Johnston
> > > wrote:
> > >> On Friday, October 2, 2020, Amit Langote
> > >> w
On Fri, Nov 20, 2020 at 8:55 PM Amit Langote wrote:
> On Sat, Oct 3, 2020 at 8:26 PM Amit Langote wrote:
> > On Sat, Oct 3, 2020 at 8:15 PM Tomas Vondra
> > wrote
> > > I think we need to differentiate between master and backbranches. IMO we
> > > should try to make it "just work" in master, an
Hi,
thanks for looking into this. I haven't yet looked at your patch in detail, yet
I think we should address the general issue here. To me this doesn't seem to be
a RI-trigger issue, but a more general issue like I mentioned in the pg-bugs
thread
https://www.postgresql.org/message-id/b1bfc99
Hi,
On Tue, Dec 15, 2020 at 12:01 AM Arne Roland wrote:
> thanks for looking into this. I haven't yet looked at your patch in detail,
> yet I think we should address the general issue here. To me this doesn't seem
> to be a RI-trigger issue, but a more general issue like I mentioned in the
> p
On Tue, Dec 15, 2020 at 12:43 PM Amit Langote wrote:
> Quoting your original example:
>
> drop table a, b;
> create table a (id serial, primary key (id)) partition by range (id);
> create table b (id serial, primary key (id)) partition by range (id);
> alter table b add constraint a_fk foreign ke
k the infrastructure to
track whether the tuple is removed or only rerouted.
Regards
Arne
From: Amit Langote
Sent: Tuesday, December 15, 2020 4:45:19 AM
To: Arne Roland
Cc: Tomas Vondra; David G. Johnston; PostgreSQL-development
Subject: Re: a misbehavior of part
Hi,
On Mon, Dec 21, 2020 at 11:30 PM Arne Roland wrote:
> thanks for the quick reply! Sadly I have been busy and the second part of
> your patch does no longer apply in src/include/nodes/execnodes.h:497.
I don't see any problem with applying the patch. Are you sure you're
applying the patch to
On Tue, Dec 22, 2020 at 4:16 PM Amit Langote wrote:
> On Mon, Dec 21, 2020 at 11:30 PM Arne Roland wrote:
> > thanks for the quick reply! Sadly I have been busy and the second part of
> > your patch does no longer apply in src/include/nodes/execnodes.h:497.
>
> I don't see any problem with apply
While I'd agree that simply deleting with "on delete cascade" on tuple
rerouting is a strong enough violation of the spirit of partitioning changing
that behavior, I am surprised by the idea to make a differentiation between fks
and other triggers. The way user defined triggers work, is a violat
Sawada-san,
On Wed, Mar 10, 2021 at 4:51 PM Masahiko Sawada wrote:
> I looked at the 0001 patch and here are random comments. Please ignore
> a comment if it is already discussed.
Thanks a lot for the review and sorry for the delay in replying.
> ---
> @@ -9077,7 +9102,8 @@ addFkRecurseReferenc
On Tue, Mar 23, 2021 at 6:27 PM Amit Langote wrote:
>
> Sawada-san,
>
> On Wed, Mar 10, 2021 at 4:51 PM Masahiko Sawada wrote:
> > I looked at the 0001 patch and here are random comments. Please ignore
> > a comment if it is already discussed.
>
> Thanks a lot for the review and sorry for the del
On Thu, Apr 1, 2021 at 10:56 AM Masahiko Sawada wrote:
> On Tue, Mar 23, 2021 at 6:27 PM Amit Langote wrote:
> > Actually, I found a big hole in my assumptions around deferrable
> > foreign constraints, invalidating the approach I took in 0002 to use a
> > query-lifetime tuplestore to record root
On Wed, Jan 20, 2021 at 7:04 PM Amit Langote wrote:
>
> On Wed, Jan 20, 2021 at 4:13 PM Peter Eisentraut
> wrote:
> > On 2021-01-08 09:54, Amit Langote wrote:
> > >>> I don't quite recall if the decision to implement it like this was
> > >>> based on assuming that this is what users would like to
Thank you for looking at this.
On Mon, Feb 15, 2021 at 4:12 PM Masahiko Sawada wrote:
> On Wed, Jan 20, 2021 at 7:04 PM Amit Langote wrote:
> > This shows that the way we've made these triggers behave in general
> > can cause some unintended behaviors for foreign keys during
> > cross-partition
Hi Amit,
>
> Here is an updated version of the patch with some cosmetic changes
> from the previous version. I moved the code being added to
> AfterTriggerSaveEvent() and ExecUpdate() into separate subroutines to
> improve readability, hopefully.
>
> I tested these patches. It works as expected
Hi Rahila,
Thanks for the review.
On Thu, Feb 18, 2021 at 7:08 PM Rahila Syed wrote:
>> Here is an updated version of the patch with some cosmetic changes
>> from the previous version. I moved the code being added to
>> AfterTriggerSaveEvent() and ExecUpdate() into separate subroutines to
>> im
On Mon, Feb 15, 2021 at 10:37 PM Amit Langote wrote:
>
> Thank you for looking at this.
>
> On Mon, Feb 15, 2021 at 4:12 PM Masahiko Sawada wrote:
> > On Wed, Jan 20, 2021 at 7:04 PM Amit Langote
> > wrote:
> > > This shows that the way we've made these triggers behave in general
> > > can caus
On Fri, Feb 19, 2021 at 5:04 PM Masahiko Sawada wrote:
> On Mon, Feb 15, 2021 at 10:37 PM Amit Langote wrote:
> > Regarding the patch, I would have liked if it only prevented the
> > update when the foreign key that would be violated by the component
> > delete references the update query's main
Hi Amit,
Sorry for the late reply.
I assume these are comments for the v3-0001 & v3-0002 patches...
>
> Yes, those were comments for patches on master.
> > The partition-key-update-1.spec test fails with the following error
> message appearing in the diffs.
> >
> > step s1u3pc: UPDATE foo_rang
On Mon, Feb 22, 2021 at 3:04 PM Amit Langote wrote:
>
> On Fri, Feb 19, 2021 at 5:04 PM Masahiko Sawada wrote:
> > On Mon, Feb 15, 2021 at 10:37 PM Amit Langote
> > wrote:
> > > Regarding the patch, I would have liked if it only prevented the
> > > update when the foreign key that would be viol
Hi Rahila,
On Wed, Feb 24, 2021 at 3:07 PM Rahila Syed wrote:
>> > I think the documentation update is missing from the patches.
>>
>> Hmm, I don't think we document the behavior that is improved by the v3
>> patches as a limitation of any existing feature, neither of foreign
>> keys referencing
On Fri, Feb 26, 2021 at 4:30 PM Amit Langote wrote:
>
> Hi Rahila,
>
> On Wed, Feb 24, 2021 at 3:07 PM Rahila Syed wrote:
> >> > I think the documentation update is missing from the patches.
> >>
> >> Hmm, I don't think we document the behavior that is improved by the v3
> >> patches as a limitat
Hi,
On Mon, Dec 28, 2020 at 10:01 PM Arne Roland wrote:
> While I'd agree that simply deleting with "on delete cascade" on tuple
> rerouting is a strong enough violation of the spirit of partitioning changing
> that behavior, I am surprised by the idea to make a differentiation between
> fks a
On 2021-01-08 09:54, Amit Langote wrote:
I don't quite recall if the decision to implement it like this was
based on assuming that this is what users would like to see happen in
this case or the perceived difficulty of implementing it the other way
around, that is, of firing AFTER UPDATE triggers
On Wed, Jan 20, 2021 at 4:13 PM Peter Eisentraut
wrote:
> On 2021-01-08 09:54, Amit Langote wrote:
> >>> I don't quite recall if the decision to implement it like this was
> >>> based on assuming that this is what users would like to see happen in
> >>> this case or the perceived difficulty of imp
On Wed, Jan 20, 2021 at 7:03 PM Amit Langote wrote:
> On Wed, Jan 20, 2021 at 4:13 PM Peter Eisentraut
> > Could you summarize here what you are trying to do with respect to what
> > was decided before? I'm a bit confused, looking through the patches you
> > have posted. The first patch you post
On Fri, Oct 2, 2020 at 11:32 PM David G. Johnston
wrote:
> On Friday, October 2, 2020, Amit Langote wrote:
>>
>>
>> Reporter on that thread says that the last update should have failed
>> and I don't quite see a workable alternative to that.
>
>
> To be clear the OP would rather have it just work
On Sat, Oct 03, 2020 at 11:42:21AM +0900, Amit Langote wrote:
On Fri, Oct 2, 2020 at 11:32 PM David G. Johnston
wrote:
On Friday, October 2, 2020, Amit Langote
wrote:
Reporter on that thread says that the last update should have failed
and I don't quite see a workable alternative to that.
On Sat, Oct 3, 2020 at 8:15 PM Tomas Vondra wrote
> On Sat, Oct 03, 2020 at 11:42:21AM +0900, Amit Langote wrote:
> >On Fri, Oct 2, 2020 at 11:32 PM David G. Johnston
> > wrote:
> >> On Friday, October 2, 2020, Amit Langote
> >> wrote:
> >>>
> >>>
> >>> Reporter on that thread says that the last
I rebased this patch; v15 attached. Other than fixing the (very large)
conflicts due to nodeModifyTable.c rework, the most important change is
moving GetAncestorResultRels into execMain.c and renaming it to have the
"Exec-" prefix. The reason is that what this code is doing is affect
struct Resul
On Fri, Mar 18, 2022 at 9:38 AM Alvaro Herrera
wrote:
> I rebased this patch; v15 attached. Other than fixing the (very large)
> conflicts due to nodeModifyTable.c rework, the most important change is
> moving GetAncestorResultRels into execMain.c and renaming it to have the
> "Exec-" prefix. T
On 2022-Mar-18, Zhihong Yu wrote:
> +#define AFTER_TRIGGER_OFFSET 0x07FF /* must be low-order
> bits */
> +#define AFTER_TRIGGER_DONE 0x8000
> +#define AFTER_TRIGGER_IN_PROGRESS 0x4000
>
> Is it better if the order of AFTER_TRIGGER_DONE
> and AFTER_TRIGGER_
On Sun, Mar 20, 2022 at 5:13 AM Alvaro Herrera wrote:
> On 2022-Mar-18, Zhihong Yu wrote:
>
> > +#define AFTER_TRIGGER_OFFSET 0x07FF /* must be low-order
> > bits */
> > +#define AFTER_TRIGGER_DONE 0x8000
> > +#define AFTER_TRIGGER_IN_PROGRESS 0x4000
> >
> >
On 2022-Mar-20, Amit Langote wrote:
> On Sun, Mar 20, 2022 at 5:13 AM Alvaro Herrera
> wrote:
> > On 2022-Mar-18, Zhihong Yu wrote:
> > > + if (!partRel->rd_rel->relispartition)
> > > + elog(ERROR, "cannot find ancestors of a non-partition result
> > > relation");
> > >
> > > It would b
Hi Alvaro,
On Mon, Mar 21, 2022 at 2:58 AM Alvaro Herrera wrote:
> On 2022-Mar-20, Amit Langote wrote:
> > On Sun, Mar 20, 2022 at 5:13 AM Alvaro Herrera
> > wrote:
> > > On 2022-Mar-18, Zhihong Yu wrote:
>
> > > > + if (!partRel->rd_rel->relispartition)
> > > > + elog(ERROR, "cannot fi
On Thu, Oct 14, 2021 at 6:00 PM Amit Langote wrote:
> On Mon, Sep 20, 2021 at 3:32 PM Amit Langote wrote:
> > The problem was that the tuplestore
> > (afterTriggers.query_stack[query_level].tuplestore) that I decided to
> > use to store the AFTER trigger tuples of a partitioned table that is
> >
Pushed 0001.
I had to adjust the query used in pg_dump; you changed the attribute
name in the query used for pg15, but left unchanged the one for older
branches, so pg_dump failed for all branches other than 15. Also,
psql's describe.c required a small tweak to a version number test.
https://gith
Hi Alvaro,
On Thu, Jan 6, 2022 at 7:27 AM Alvaro Herrera wrote:
> Pushed 0001.
Thank you.
> I had to adjust the query used in pg_dump; you changed the attribute
> name in the query used for pg15, but left unchanged the one for older
> branches, so pg_dump failed for all branches other than 15.
On 2022-Jan-06, Amit Langote wrote:
> On Thu, Jan 6, 2022 at 7:27 AM Alvaro Herrera wrote:
> > I have pushed it thinking that we would not backpatch any of this fix.
> > However, after running the tests and realizing that I didn't need an
> > initdb for either patch, I wonder if maybe the whole
On Thu, Jan 6, 2022 at 9:36 PM Alvaro Herrera wrote:
> On 2022-Jan-06, Amit Langote wrote:
> > On Thu, Jan 6, 2022 at 7:27 AM Alvaro Herrera
> > wrote:
>
> > > I have pushed it thinking that we would not backpatch any of this fix.
> > > However, after running the tests and realizing that I didn'
On 2022-Jan-11, Amit Langote wrote:
> As for the fix to make cross-partition updates work correctly with
> foreign keys, I just realized it won't work for the users' existing
> foreign keys, because the parent table's triggers that are needed for
> the fix to work would not be present. Were you t
Hi,
On Tue, Jan 11, 2022 at 05:08:59PM +0900, Amit Langote wrote:
>
> I think I've managed to apply f4566345cf40b into v13 and v14. Patches
> attached.
>
FTR this doesn't play well with the cfbot unfortunately as it tries to apply
both patches, and obviously on the wrong branches anyway.
It
On Thu, Jan 13, 2022 at 12:19 PM Julien Rouhaud wrote:
> On Tue, Jan 11, 2022 at 05:08:59PM +0900, Amit Langote wrote:
> >
> > I think I've managed to apply f4566345cf40b into v13 and v14. Patches
> > attached.
> >
>
> FTR this doesn't play well with the cfbot unfortunately as it tries to apply
On 2022-Jan-17, Amit Langote wrote:
> Note that the fix involves adding fields to ResultRelInfo -- v13 needs
> 2 additional, while v14 and HEAD need 1. That combined with needing
> new catalog entries for parent FK triggers, back-patching this does
> make me a bit uncomfortable.
Yeah, that's a g
On Mon, Jan 17, 2022 at 6:26 AM Alvaro Herrera
wrote:
> On 2022-Jan-17, Amit Langote wrote:
>
> > Note that the fix involves adding fields to ResultRelInfo -- v13 needs
> > 2 additional, while v14 and HEAD need 1. That combined with needing
> > new catalog entries for parent FK triggers, back-pa
On 2022-Jan-17, Zhihong Yu wrote:
> On Mon, Jan 17, 2022 at 6:26 AM Alvaro Herrera
> wrote:
> > On 2022-Jan-17, Amit Langote wrote:
> > The other is suggested by you:
> >
> > > Another thing to consider is that we haven't seen many reports of the
> > > problem (UPDATEs of partitioned PK tables
> @@ -3398,7 +3432,7 @@ typedef SetConstraintStateData *SetConstraintState;
> */
> typedef uint32 TriggerFlags;
>
> -#define AFTER_TRIGGER_OFFSET 0x0FFF /* must be
> low-order bits */
> +#define AFTER_TRIGGER_OFFSET 0x07FF /* must be
> low-
Alvaro Herrera writes:
> So this patch releases one bit from AFTER_TRIGGER_OFFSET and makes it
> become AFTER_TRIGGER_CP_UPDATE. As far as I can tell there is no harm
> in doing so.
I agree that taking a bit away from AFTER_TRIGGER_OFFSET is okay
(it could spare even a couple more, if we need th
On 2022-Jan-17, Tom Lane wrote:
> But could we please do it in a way that is designed to keep the
> code readable, rather than to minimize the number of lines of diff?
> It makes zero sense to have the bits in AFTER_TRIGGER_TUP_BITS not
> be adjacent. So what should happen here is to renumber the
Alvaro Herrera writes:
> On 2022-Jan-17, Tom Lane wrote:
>> It makes zero sense to have the bits in AFTER_TRIGGER_TUP_BITS not
>> be adjacent. So what should happen here is to renumber the symbols
>> in between to move their bits over one place.
> Is it typical to enumerate bits starting from th
On Tue, Jan 18, 2022 at 7:15 AM Alvaro Herrera wrote:
> On 2022-Jan-17, Tom Lane wrote:
> > But could we please do it in a way that is designed to keep the
> > code readable, rather than to minimize the number of lines of diff?
> > It makes zero sense to have the bits in AFTER_TRIGGER_TUP_BITS not
Hi,
On Mon, Jan 17, 2022 at 08:40:54PM +0900, Amit Langote wrote:
>
> Okay, I created versions of the patch series for branches 13 and 14
> (.txt files). The one for HEAD is also re-attached.
FYI The patch failed today on FreeBSD, while it was previously quite stable on
all platforms (https://c
On Tue, Jan 18, 2022 at 12:16:23PM +0800, Julien Rouhaud wrote:
> I'm not sure why this test failed as it doesn't seem like something impacted
> by
> the patch, but I may have missed something as I only had a quick look at the
> patch and discussion.
This issue is discussed here:
https://www.post
Hi,
On Tue, Jan 18, 2022 at 02:33:39PM +0900, Michael Paquier wrote:
> On Tue, Jan 18, 2022 at 12:16:23PM +0800, Julien Rouhaud wrote:
> > I'm not sure why this test failed as it doesn't seem like something
> > impacted by
> > the patch, but I may have missed something as I only had a quick look
On Tue, Jan 18, 2022 at 2:41 PM Julien Rouhaud wrote:
> On Tue, Jan 18, 2022 at 02:33:39PM +0900, Michael Paquier wrote:
> > On Tue, Jan 18, 2022 at 12:16:23PM +0800, Julien Rouhaud wrote:
> > > I'm not sure why this test failed as it doesn't seem like something
> > > impacted by
> > > the patch,
On 2022-Jan-18, Amit Langote wrote:
> Would you like me to update the patch with the above renumbering or
> are you working on a new version yourself?
I have a few very minor changes apart from that. Let me see if I can
get this pushed soon; if I'm unable to (there are parts I don't fully
grok y
Julien Rouhaud writes:
> @@ -133,7 +133,7 @@
> SELECT pg_relation_size('reloptions_test') = 0;
> ?column?
> --
> - t
> + f
> (1 row)
Some machines have been showing that on HEAD too, so I doubt
it's the fault of this patch. That reloptions test isn't stable
yet seemingly.
On 2022-Jan-18, Alvaro Herrera wrote:
> On 2022-Jan-18, Amit Langote wrote:
>
> > Would you like me to update the patch with the above renumbering or
> > are you working on a new version yourself?
>
> I have a few very minor changes apart from that. Let me see if I can
> get this pushed soon; i
On Wed, Jan 19, 2022 at 7:29 AM Alvaro Herrera wrote:
> On 2022-Jan-18, Alvaro Herrera wrote:
> > On 2022-Jan-18, Amit Langote wrote:
> >
> > > Would you like me to update the patch with the above renumbering or
> > > are you working on a new version yourself?
> >
> > I have a few very minor chang
On Wed, Jan 19, 2022 at 4:13 PM Amit Langote wrote:
> On Wed, Jan 19, 2022 at 7:29 AM Alvaro Herrera
> wrote:
> > On 2022-Jan-18, Alvaro Herrera wrote:
> > > On 2022-Jan-18, Amit Langote wrote:
> > >
> > > > Would you like me to update the patch with the above renumbering or
> > > > are you work
On 2022-Jan-19, Amit Langote wrote:
> BTW, your tweaks patch added this:
>
> + * *inserted_tuple is the tuple that's effectively inserted;
> + * *inserted_destrel is the relation where it was inserted.
> + * These are only set on success. FIXME -- see what happens on
> the "do nothin
On Wed, Jan 19, 2022 at 6:26 PM Alvaro Herrera wrote:
> On 2022-Jan-19, Amit Langote wrote:
> > BTW, your tweaks patch added this:
> >
> > + * *inserted_tuple is the tuple that's effectively inserted;
> > + * *inserted_destrel is the relation where it was inserted.
> > + * These are on
On 7/13/21 8:09 AM, Amit Langote wrote:
>
>
>
> @Amit patch is not successfully applying, can you please rebase that.
>
>
> Thanks for the reminder.
>
> Masahiko Sawada, it's been a bit long since you reviewed the
> patch, are you still interested to review that?
>
>
> Unfortunately
Hi Andrew,
On Fri, Sep 3, 2021 at 6:19 AM Andrew Dunstan wrote:
> On 7/13/21 8:09 AM, Amit Langote wrote:
> > Unfortunately, I don’t think I’ll have time in this CF to solve some
> > very fundamental issues I found in the patch during the last cycle.
> > I’m fine with either marking this as RwF f
On Fri, Sep 3, 2021 at 12:23 PM Amit Langote wrote:
> Hi Andrew,
>
> On Fri, Sep 3, 2021 at 6:19 AM Andrew Dunstan wrote:
> > On 7/13/21 8:09 AM, Amit Langote wrote:
> > > Unfortunately, I don’t think I’ll have time in this CF to solve some
> > > very fundamental issues I found in the patch durin
On Fri, Sep 10, 2021 at 7:06 AM Amit Langote
wrote:
> On Fri, Sep 3, 2021 at 12:23 PM Amit Langote
> wrote:
> > Hi Andrew,
> >
> > On Fri, Sep 3, 2021 at 6:19 AM Andrew Dunstan
> wrote:
> > > On 7/13/21 8:09 AM, Amit Langote wrote:
> > > > Unfortunately, I don’t think I’ll have time in this CF
On Fri, Sep 10, 2021 at 11:03 PM Amit Langote wrote:
> On Fri, Sep 3, 2021 at 12:23 PM Amit Langote wrote:
> > Hi Andrew,
> >
> > On Fri, Sep 3, 2021 at 6:19 AM Andrew Dunstan wrote:
> > > On 7/13/21 8:09 AM, Amit Langote wrote:
> > > > Unfortunately, I don’t think I’ll have time in this CF to s
On Mon, Sep 20, 2021 at 3:32 PM Amit Langote wrote:
> The problem was that the tuplestore
> (afterTriggers.query_stack[query_level].tuplestore) that I decided to
> use to store the AFTER trigger tuples of a partitioned table that is
> the target of an cross-partition update lives only for the dura
On Fri, Apr 2, 2021 at 6:09 PM Amit Langote wrote:
> On Thu, Apr 1, 2021 at 10:56 AM Masahiko Sawada
> wrote:
> > On Tue, Mar 23, 2021 at 6:27 PM Amit Langote
> wrote:
> > > Actually, I found a big hole in my assumptions around deferrable
> > > foreign constraints, invalidating the approach I t
Hi Ibrar, Sawada-san,
On Tue, Jul 13, 2021 at 20:25 Ibrar Ahmed wrote:
>
>
> On Fri, Apr 2, 2021 at 6:09 PM Amit Langote
> wrote:
>
>> On Thu, Apr 1, 2021 at 10:56 AM Masahiko Sawada
>> wrote:
>> > On Tue, Mar 23, 2021 at 6:27 PM Amit Langote
>> wrote:
>> > > Actually, I found a big hole in m
70 matches
Mail list logo
