Subject: Re: a misbehavior of partition row movement (?)

From: Alvaro Herrera <[email-masked]>
Date: Mon, 17 Jan 2022 18:22:19 -0300

> @@ -3398,7 +3432,7 @@ typedef SetConstraintStateData *SetConstraintState;
>   */
>  typedef uint32 TriggerFlags;
>  
> -#define AFTER_TRIGGER_OFFSET			0x0FFFFFFF	/* must be low-order bits */
> +#define AFTER_TRIGGER_OFFSET			0x07FFFFFF	/* must be low-order bits */
>  #define AFTER_TRIGGER_DONE				0x10000000
>  #define AFTER_TRIGGER_IN_PROGRESS		0x20000000
>  /* bits describing the size and tuple sources of this event */
> @@ -3406,7 +3440,8 @@ typedef uint32 TriggerFlags;
>  #define AFTER_TRIGGER_FDW_FETCH			0x80000000
>  #define AFTER_TRIGGER_1CTID				0x40000000
>  #define AFTER_TRIGGER_2CTID				0xC0000000
> -#define AFTER_TRIGGER_TUP_BITS			0xC0000000
> +#define AFTER_TRIGGER_CP_UPDATE			0x08000000
> +#define AFTER_TRIGGER_TUP_BITS			0xC8000000

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.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

---

From: Tom Lane <[email-masked]>
Date: Mon, 17 Jan 2022 16:38:05 -0500

> become AFTER_TRIGGER_CP_UPDATE.  As far as I can tell there is no harm

I agree that taking a bit away from AFTER_TRIGGER_OFFSET is okay
(it could spare even a couple more, if we need them).

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 symbols
in between to move their bits over one place.

(Since this data is only known within trigger.c, I don't even see
an ABI-stability argument for not changing these assignments.)

			regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 17 Jan 2022 19:15:40 -0300

> in between to move their bits over one place.

Is it typical to enumerate bits starting from the right of each byte,
when doing it from the high bits of the word?  DONE and IN_PROGRESS have
been defined as 0x1 and 0x2 of that byte for a very long time and I
found that very strange.  I am inclined to count from the left, so I'd
pick 8 first, defining the set like this:

#define AFTER_TRIGGER_OFFSET			0x07FFFFFF	/* must be low-order bits */
#define AFTER_TRIGGER_DONE			0x80000000
#define AFTER_TRIGGER_IN_PROGRESS		0x40000000
/* bits describing the size and tuple sources of this event */
#define AFTER_TRIGGER_FDW_REUSE			0x00000000
#define AFTER_TRIGGER_FDW_FETCH			0x20000000
#define AFTER_TRIGGER_1CTID			0x10000000
#define AFTER_TRIGGER_2CTID			0x30000000
#define AFTER_TRIGGER_CP_UPDATE			0x08000000
#define AFTER_TRIGGER_TUP_BITS			0x38000000

(The fact that FDW_REUSE bits is actually an empty mask comes from
7cbe57c34dec, specifically [1])

Is this what you were thinking?

[1] https://postgr.es/m/[email-masked]

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"All rings of power are equal,
But some rings of power are more equal than others."
                                 (George Orwell's The Lord of the Rings)

---

From: Tom Lane <[email-masked]>
Date: Mon, 17 Jan 2022 18:11:22 -0500

> pick 8 first, defining the set like this:

Doesn't matter to me either way, as long as the values look like they
were all defined by the same person ;-)

> 7cbe57c34dec, specifically [1])

That seemed a bit odd to me too, but this is not the patch to be
changing it in, I suppose.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Tue, 18 Jan 2022 12:00:45 +0900

> #define AFTER_TRIGGER_TUP_BITS                  0x38000000

Agree that the ultimate code readability trumps diff minimalism. :-)

Would you like me to update the patch with the above renumbering or
are you working on a new version yourself?

-- 
Amit Langote
EDB: http://www.enterprisedb.com

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 18 Jan 2022 09:22:55 -0300

> 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 yet), I'll post what I have.

Thank you!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/