> On May 14, 2020, at 10:40 AM, Ranier Vilela <ranier.vf@gmail.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.