Обсуждение: Remove unused fields in ReorderBufferTupleBuf

Hi,

ReorderBufferTupleBuf is defined as follow:

/* an individual tuple, stored in one chunk of memory */
typedef struct ReorderBufferTupleBuf
{
    /* position in preallocated list */
    slist_node  node;

    /* tuple header, the interesting bit for users of logical decoding */
    HeapTupleData tuple;

    /* pre-allocated size of tuple buffer, different from tuple size */
    Size        alloc_tuple_size;

    /* actual tuple data follows */
} ReorderBufferTupleBuf;

However, node and alloc_tuple_size are not used at all. It seems an
oversight in commit a4ccc1cef5a, which introduced the generation
context and used it in logical decoding. I think we can remove them
(only on HEAD). I've attached the patch.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Hi,

Thanks for the patch.

> However, node and alloc_tuple_size are not used at all. It seems an
> oversight in commit a4ccc1cef5a, which introduced the generation
> context and used it in logical decoding. I think we can remove them
> (only on HEAD). I've attached the patch.

I'm afraid it's a bit incomplete:

```
../src/backend/replication/logical/reorderbuffer.c: In function
‘ReorderBufferGetTupleBuf’:
../src/backend/replication/logical/reorderbuffer.c:565:14: error:
‘ReorderBufferTupleBuf’ has no member named ‘alloc_tuple_size’
  565 |         tuple->alloc_tuple_size = alloc_len;
      |              ^~
[829/1882] Compiling C object contrib/xml2/pgxml.so.p/xpath.c.o
```

On top of that IMO it doesn't make much sense to keep a one-field
wrapper structure. Perhaps we should get rid of it entirely and just
use HeapTupleData instead.

Thoughts?

--
Best regards,
Aleksander Alekseev

Hi,

> I'm afraid it's a bit incomplete:
>
> ```
> ../src/backend/replication/logical/reorderbuffer.c: In function
> ‘ReorderBufferGetTupleBuf’:
> ../src/backend/replication/logical/reorderbuffer.c:565:14: error:
> ‘ReorderBufferTupleBuf’ has no member named ‘alloc_tuple_size’
>   565 |         tuple->alloc_tuple_size = alloc_len;
>       |              ^~
> [829/1882] Compiling C object contrib/xml2/pgxml.so.p/xpath.c.o
> ```

Here is the corrected patch. I added it to the nearest CF [1].

> On top of that IMO it doesn't make much sense to keep a one-field
> wrapper structure. Perhaps we should get rid of it entirely and just
> use HeapTupleData instead.
>
> Thoughts?

Alternatively we could convert ReorderBufferTupleBufData macro to an
inlined function. At least it will add some type safety.

I didn't change anything in this respect in v2. Feedback from the
community would be much appreciated.

[1]: https://commitfest.postgresql.org/44/4461/

--
Best regards,
Aleksander Alekseev

Hi,

> Here is the corrected patch. I added it to the nearest CF [1].

I played a bit more with the patch. There was an idea to make
ReorderBufferTupleBufData an opaque structure known only within
reorderbyffer.c but it turned out that replication/logical/decode.c
accesses it directly so I abandoned that idea for now.

> Alternatively we could convert ReorderBufferTupleBufData macro to an
> inlined function. At least it will add some type safety.

Here is v3 that implements it too as a separate patch.

Apologies for the noise.

-- 
Best regards,
Aleksander Alekseev

2024-01 Commitfest.

Hi, this patch was marked in CF as "Needs Review" [1], but there has
been no activity on this thread for 5+ months.

Do you wish to keep this open, or can you post something to elicit
more interest in reviews for the latest patch set? Otherwise, if
nothing happens then the CF entry will be closed ("Returned with
feedback") at the end of this CF.

======
[1] https://commitfest.postgresql.org/46/4461/

Kind Regards,
Peter Smith.

Hi,

> Hi, this patch was marked in CF as "Needs Review" [1], but there has
> been no activity on this thread for 5+ months.
>
> Do you wish to keep this open, or can you post something to elicit
> more interest in reviews for the latest patch set? Otherwise, if
> nothing happens then the CF entry will be closed ("Returned with
> feedback") at the end of this CF.

I don't think that closing CF entries only due to inactivity is a good
practice, nor something we typically do. When someone will have spare
time this person will (hopefully) review the code.

-- 
Best regards,
Aleksander Alekseev

On Wed, Jul 26, 2023 at 7:22 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> > Here is the corrected patch. I added it to the nearest CF [1].
>
> I played a bit more with the patch. There was an idea to make
> ReorderBufferTupleBufData an opaque structure known only within
> reorderbyffer.c but it turned out that replication/logical/decode.c
> accesses it directly so I abandoned that idea for now.
>
> > Alternatively we could convert ReorderBufferTupleBufData macro to an
> > inlined function. At least it will add some type safety.
>
> Here is v3 that implements it too as a separate patch.
>

But why didn't you pursue your idea of getting rid of the wrapper
structure ReorderBufferTupleBuf which after this patch will have just
one member? I think there could be hassles in backpatching bug-fixes
in some cases but in the longer run it would make the code look clean.

--
With Regards,
Amit Kapila.

On Mon, Jan 22, 2024 at 4:17 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > Hi, this patch was marked in CF as "Needs Review" [1], but there has
> > been no activity on this thread for 5+ months.
> >
> > Do you wish to keep this open, or can you post something to elicit
> > more interest in reviews for the latest patch set? Otherwise, if
> > nothing happens then the CF entry will be closed ("Returned with
> > feedback") at the end of this CF.
>
> I don't think that closing CF entries only due to inactivity is a good
> practice, nor something we typically do. When someone will have spare
> time this person will (hopefully) review the code.

Agree. IMHO, a patch not picked up by anyone doesn't mean it's the
author's problem to mark it "Returned with feedback". And, the timing
to mark things this way is not quite right as we are getting close to
the PG17 release. The patch may be fixing a problem (like the one
that's closed due to inactivity
https://commitfest.postgresql.org/46/3503/) or may be a feature or an
improvement that no reviewer/committer has had a chance to look at.

I think a new label such as "Returned due to lack of interest" or
"Returned due to disinterest" (or some better wording) helps
reviewers/committers to pick things up. This new label can be
attributed to the class of patches for which initially there's some
positive feedback on the idea, the patch is being kept latest, but
finds no one to get it reviewed for say X number of months.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

> > I played a bit more with the patch. There was an idea to make
> > ReorderBufferTupleBufData an opaque structure known only within
> > reorderbyffer.c but it turned out that replication/logical/decode.c
> > accesses it directly so I abandoned that idea for now.
> >
> > > Alternatively we could convert ReorderBufferTupleBufData macro to an
> > > inlined function. At least it will add some type safety.
> >
> > Here is v3 that implements it too as a separate patch.
> >
>
> But why didn't you pursue your idea of getting rid of the wrapper
> structure ReorderBufferTupleBuf which after this patch will have just
> one member? I think there could be hassles in backpatching bug-fixes
> in some cases but in the longer run it would make the code look clean.

Indeed. In fact turned out that I suggested the same above but
apparently forgot:

> On top of that IMO it doesn't make much sense to keep a one-field
> wrapper structure. Perhaps we should get rid of it entirely and just
> use HeapTupleData instead.

After actually trying the refactoring I agree that the code becomes
cleaner and it's going to be beneficial in the long run. Here is the
patch.

--
Best regards,
Aleksander Alekseev

Hi,

> > But why didn't you pursue your idea of getting rid of the wrapper
> > structure ReorderBufferTupleBuf which after this patch will have just
> > one member? I think there could be hassles in backpatching bug-fixes
> > in some cases but in the longer run it would make the code look clean.
>
> Indeed. In fact turned out that I suggested the same above but
> apparently forgot:
>
> > On top of that IMO it doesn't make much sense to keep a one-field
> > wrapper structure. Perhaps we should get rid of it entirely and just
> > use HeapTupleData instead.
>
> After actually trying the refactoring I agree that the code becomes
> cleaner and it's going to be beneficial in the long run. Here is the
> patch.

I did a mistake in v4:

```
-    alloc_len = tuple_len + SizeofHeapTupleHeader;
+    alloc_len = tuple_len + HEAPTUPLESIZE;
```

Here is the corrected patch.

-- 
Best regards,
Aleksander Alekseev

Sorry for being absent for a while.

On Mon, Jan 22, 2024 at 11:19 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > > But why didn't you pursue your idea of getting rid of the wrapper
> > > structure ReorderBufferTupleBuf which after this patch will have just
> > > one member? I think there could be hassles in backpatching bug-fixes
> > > in some cases but in the longer run it would make the code look clean.

+1

> >
> > Indeed. In fact turned out that I suggested the same above but
> > apparently forgot:
> >
> > > On top of that IMO it doesn't make much sense to keep a one-field
> > > wrapper structure. Perhaps we should get rid of it entirely and just
> > > use HeapTupleData instead.
> >
> > After actually trying the refactoring I agree that the code becomes
> > cleaner and it's going to be beneficial in the long run. Here is the
> > patch.
>
> I did a mistake in v4:
>
> ```
> -    alloc_len = tuple_len + SizeofHeapTupleHeader;
> +    alloc_len = tuple_len + HEAPTUPLESIZE;
> ```
>
> Here is the corrected patch.

Thank you for updating the patch! I have some comments:

-        tuple = (ReorderBufferTupleBuf *)
+        tuple = (HeapTuple)
                 MemoryContextAlloc(rb->tup_context,
-
sizeof(ReorderBufferTupleBuf) +
+                                                   HEAPTUPLESIZE +
                                                    MAXIMUM_ALIGNOF +
alloc_len);
-        tuple->alloc_tuple_size = alloc_len;
-        tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
+        tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);

Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
similar thing but it doesn't add it.

---
                                 xl_parameter_change *xlrec =
-                                        (xl_parameter_change *)
XLogRecGetData(buf->record);
+                                (xl_parameter_change *)
XLogRecGetData(buf->record);

Unnecessary change.

---
-/*
- * Free a ReorderBufferTupleBuf.
- */
-void
-ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
-{
-        pfree(tuple);
-}
-

Why does ReorderBufferReturnTupleBuf need to be moved from
reorderbuffer.c to reorderbuffer.h? It seems not related to this
refactoring patch so I think we should do it in a separate patch if we
really want it. I'm not sure it's necessary, though.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Hi,

> > Here is the corrected patch.
>
> Thank you for updating the patch! I have some comments:

Thanks for the review.

> -        tuple = (ReorderBufferTupleBuf *)
> +        tuple = (HeapTuple)
>                  MemoryContextAlloc(rb->tup_context,
> -
> sizeof(ReorderBufferTupleBuf) +
> +                                                   HEAPTUPLESIZE +
>                                                     MAXIMUM_ALIGNOF +
> alloc_len);
> -        tuple->alloc_tuple_size = alloc_len;
> -        tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
> +        tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);
>
> Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
> MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
> similar thing but it doesn't add it.

Indeed. I gave it a try and nothing crashed, so it appears that
MAXIMUM_ALIGNOF is not needed.

> ---
>                                  xl_parameter_change *xlrec =
> -                                        (xl_parameter_change *)
> XLogRecGetData(buf->record);
> +                                (xl_parameter_change *)
> XLogRecGetData(buf->record);
>
> Unnecessary change.

That's pgindent. Fixed.

> ---
> -/*
> - * Free a ReorderBufferTupleBuf.
> - */
> -void
> -ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
> -{
> -        pfree(tuple);
> -}
> -
>
> Why does ReorderBufferReturnTupleBuf need to be moved from
> reorderbuffer.c to reorderbuffer.h? It seems not related to this
> refactoring patch so I think we should do it in a separate patch if we
> really want it. I'm not sure it's necessary, though.

OK, fixed.

-- 
Best regards,
Aleksander Alekseev

On Thu, 2024-01-25 at 16:16 +0300, Aleksander Alekseev wrote:
> Hi,
>
> > > Here is the corrected patch.
> >
> > Thank you for updating the patch! I have some comments:
>
> Thanks for the review.
>
> > -        tuple = (ReorderBufferTupleBuf *)
> > +        tuple = (HeapTuple)
> >                  MemoryContextAlloc(rb->tup_context,
> > -
> > sizeof(ReorderBufferTupleBuf) +
> > +                                                   HEAPTUPLESIZE +
> >                                                     MAXIMUM_ALIGNOF +
> > alloc_len);
> > -        tuple->alloc_tuple_size = alloc_len;
> > -        tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
> > +        tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);
> >
> > Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
> > MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
> > similar thing but it doesn't add it.
>
> Indeed. I gave it a try and nothing crashed, so it appears that
> MAXIMUM_ALIGNOF is not needed.
>
> > ---
> >                                  xl_parameter_change *xlrec =
> > -                                        (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> > +                                (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> >
> > Unnecessary change.
>
> That's pgindent. Fixed.
>
> > ---
> > -/*
> > - * Free a ReorderBufferTupleBuf.
> > - */
> > -void
> > -ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
> > -{
> > -        pfree(tuple);
> > -}
> > -
> >
> > Why does ReorderBufferReturnTupleBuf need to be moved from
> > reorderbuffer.c to reorderbuffer.h? It seems not related to this
> > refactoring patch so I think we should do it in a separate patch if we
> > really want it. I'm not sure it's necessary, though.
>
> OK, fixed.
>

I walked through v6 and didn't note any issues.

I do want to ask, the patch alters ReorderBufferReturnTupleBuf() and
drops the unused parameter ReorderBuffer *rb. It seems that
ReorderBufferFreeSnap(), ReorderBufferReturnRelids(),
ReorderBufferImmediateInvalidation(), and ReorderBufferRestoreCleanup()
also pass ReorderBuffer *rb but do not use it. Would it be beneficial
to implement a separate patch to remove this parameter from these
functions also?

Thanks,
Reid

On Thu, 2024-01-25 at 16:11 -0500, reid.thompson@crunchydata.com wrote:
>
> I walked through v6 and didn't note any issues.
>
> I do want to ask, the patch alters ReorderBufferReturnTupleBuf() and
> drops the unused parameter ReorderBuffer *rb. It seems that
> ReorderBufferFreeSnap(), ReorderBufferReturnRelids(),
> ReorderBufferImmediateInvalidation(), and ReorderBufferRestoreCleanup()
> also pass ReorderBuffer *rb but do not use it. Would it be beneficial
> to implement a separate patch to remove this parameter from these
> functions also?
>
> Thanks,
> Reid
>

It also appears that ReorderBufferSerializeChange() has 5 instances
where it increments the local variables "data" but then they're never
read.
Lines 3806, 3836, 3854, 3889, 3910

I can create patch and post it to this thread or a new one if deemed
worthwhile.

Thanks,
Reid

On Fri, Jan 26, 2024 at 1:24 PM <reid.thompson@crunchydata.com> wrote:
>
> On Thu, 2024-01-25 at 16:11 -0500, reid.thompson@crunchydata.com wrote:
> >
> > I walked through v6 and didn't note any issues.

Thank you for reviewing the patch!

> >
> > I do want to ask, the patch alters ReorderBufferReturnTupleBuf() and
> > drops the unused parameter ReorderBuffer *rb. It seems that
> > ReorderBufferFreeSnap(), ReorderBufferReturnRelids(),
> > ReorderBufferImmediateInvalidation(), and ReorderBufferRestoreCleanup()
> > also pass ReorderBuffer *rb but do not use it. Would it be beneficial
> > to implement a separate patch to remove this parameter from these
> > functions also?
> >
> >
>
> It also appears that ReorderBufferSerializeChange() has 5 instances
> where it increments the local variables "data" but then they're never
> read.
> Lines 3806, 3836, 3854, 3889, 3910
>
> I can create patch and post it to this thread or a new one if deemed
> worthwhile.

I'm not sure these changes are really beneficial. They contribute to
improving neither readability and performance IMO.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 25, 2024 at 10:17 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > > Here is the corrected patch.
> >
> > Thank you for updating the patch! I have some comments:
>
> Thanks for the review.
>
> > -        tuple = (ReorderBufferTupleBuf *)
> > +        tuple = (HeapTuple)
> >                  MemoryContextAlloc(rb->tup_context,
> > -
> > sizeof(ReorderBufferTupleBuf) +
> > +                                                   HEAPTUPLESIZE +
> >                                                     MAXIMUM_ALIGNOF +
> > alloc_len);
> > -        tuple->alloc_tuple_size = alloc_len;
> > -        tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
> > +        tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);
> >
> > Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
> > MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
> > similar thing but it doesn't add it.
>
> Indeed. I gave it a try and nothing crashed, so it appears that
> MAXIMUM_ALIGNOF is not needed.
>
> > ---
> >                                  xl_parameter_change *xlrec =
> > -                                        (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> > +                                (xl_parameter_change *)
> > XLogRecGetData(buf->record);
> >
> > Unnecessary change.
>
> That's pgindent. Fixed.
>
> > ---
> > -/*
> > - * Free a ReorderBufferTupleBuf.
> > - */
> > -void
> > -ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
> > -{
> > -        pfree(tuple);
> > -}
> > -
> >
> > Why does ReorderBufferReturnTupleBuf need to be moved from
> > reorderbuffer.c to reorderbuffer.h? It seems not related to this
> > refactoring patch so I think we should do it in a separate patch if we
> > really want it. I'm not sure it's necessary, though.
>
> OK, fixed.

Thank you for updating the patch. It looks good to me. I'm going to
push it next week, barring any objections.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Fri, 2024-01-26 at 13:51 +0900, Masahiko Sawada wrote:
> On Fri, Jan 26, 2024 at 1:24 PM <reid.thompson@crunchydata.com> wrote:
> >
> > On Thu, 2024-01-25 at 16:11 -0500, reid.thompson@crunchydata.com wrote:
> > >
> > > I walked through v6 and didn't note any issues.
>
> Thank you for reviewing the patch!
>
Happy to.
>
> I'm not sure these changes are really beneficial. They contribute to
> improving neither readability and performance IMO.
>
> Regards,
>
I thought that may be the case, but wanted to ask to be sure. Thank you
for the followup.

Reid

On Fri, Jan 26, 2024 at 4:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Jan 25, 2024 at 10:17 PM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> >
> > Hi,
> >
> > > > Here is the corrected patch.
> > >
> > > Thank you for updating the patch! I have some comments:
> >
> > Thanks for the review.
> >
> > > -        tuple = (ReorderBufferTupleBuf *)
> > > +        tuple = (HeapTuple)
> > >                  MemoryContextAlloc(rb->tup_context,
> > > -
> > > sizeof(ReorderBufferTupleBuf) +
> > > +                                                   HEAPTUPLESIZE +
> > >                                                     MAXIMUM_ALIGNOF +
> > > alloc_len);
> > > -        tuple->alloc_tuple_size = alloc_len;
> > > -        tuple->tuple.t_data = ReorderBufferTupleBufData(tuple);
> > > +        tuple->t_data = (HeapTupleHeader)((char *)tuple + HEAPTUPLESIZE);
> > >
> > > Why do we need to add MAXIMUM_ALIGNOF? since HEAPTUPLESIZE is the
> > > MAXALIGN'd size, it seems we don't need it. heap_form_tuple() does a
> > > similar thing but it doesn't add it.
> >
> > Indeed. I gave it a try and nothing crashed, so it appears that
> > MAXIMUM_ALIGNOF is not needed.
> >
> > > ---
> > >                                  xl_parameter_change *xlrec =
> > > -                                        (xl_parameter_change *)
> > > XLogRecGetData(buf->record);
> > > +                                (xl_parameter_change *)
> > > XLogRecGetData(buf->record);
> > >
> > > Unnecessary change.
> >
> > That's pgindent. Fixed.
> >
> > > ---
> > > -/*
> > > - * Free a ReorderBufferTupleBuf.
> > > - */
> > > -void
> > > -ReorderBufferReturnTupleBuf(ReorderBuffer *rb, ReorderBufferTupleBuf *tuple)
> > > -{
> > > -        pfree(tuple);
> > > -}
> > > -
> > >
> > > Why does ReorderBufferReturnTupleBuf need to be moved from
> > > reorderbuffer.c to reorderbuffer.h? It seems not related to this
> > > refactoring patch so I think we should do it in a separate patch if we
> > > really want it. I'm not sure it's necessary, though.
> >
> > OK, fixed.
>
> Thank you for updating the patch. It looks good to me. I'm going to
> push it next week, barring any objections.

Pushed (08e6344fd642).

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com