Re: [PATCH] Fix ouside scope t_ctid (ItemPointerData)

Em qui., 14 de mai. de 2020 às 15:03, Mark Dilger <
mark(dot)dilger(at)enterprisedb(dot)com> escreveu:

>
>
> > On May 14, 2020, at 10:40 AM, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> >
> > Hi,
> > ItemPointerData, on the contrary, from what the name says,
> > it is not a pointer to a structure, but a structure in fact.
>
> The project frequently uses the pattern
>
> typedef struct FooData {
> ...
> } FooData;
>
> typedef FooData *Foo;
>
> where, in this example, "Foo" = "ItemPointer".
>
> So the "Data" part of "ItemPointerData" clues the reader into the fact
> that ItemPointerData is a struct, not a pointer. Granted, the "Pointer"
> part of that name may confuse some readers, but the struct itself does
> contain what is essentially a 48-bit pointer, so that name is not nuts.
>
>
> > When assigning the name of the structure variable to a pointer, it may
> even work,
> > but, it is not the right thing to do and it becomes a nightmare,
> > to discover that any other error they have is at cause.
>
> Can you give a code example of the type of assigment you mean?
>
htup->t_ctid = target_tid;
htup->t_ctid = newtid;
Both target_tid and newtid are local variable, whe loss scope, memory is
garbage.

>
> > So:
> > 1. In some cases, there may be a misunderstanding in the use of
> ItemPointerData.
> > 2. When using the variable name in an assignment, the variable's address
> is used.
> > 3. While this works for a structure, it shouldn't be the right thing to
> do.
> > 4. If we have a local variable, its scope is limited and when it loses
> its scope, memory is certainly garbage.
> > 5. While this may be working for heapam.c, I believe it is being abused
> and should be compliant with
> > the Postgres API and use the functions that were created for this.
> >
> > The patch is primarily intended to correct the use of ItemPointerData.
> > But it is also changing the style, reducing the scope of some variables.
> > If that was not acceptable, reduce the scope and someone has objections,
> > I can change the patch, to focus only on the use of ItemPointerData.
> > But as style changes are rare, if possible, it would be good to seize
> the opportunity.
>
> I would like to see a version of the patch that only addresses your
> concerns about ItemPointerData, not because other aspects of the patch are
> unacceptable (I'm not ready to even contemplate that yet), but because I'm
> not sure what your complaint is about. Can you restrict the patch to just
> address that one issue?
>
Certainly.
In the same file you can find the appropriate use of the API.
ItemPointerSet(&heapTuple->t_self, blkno, offnum);

regards,
Ranier Vilela