On Fri, Jan 28, 2022 at 2:20 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> PG_CONTROL_VERSION is different from catversion. You should update it in this
> patch.
My bad. Updated it.
> But Horiguchi-san was also mentioning that pg_upgrade/controldata.c needs some
> modifications if you change the format (thus the requirement to bump
> PG_CONTROL_VERSION).
> Also, you still didn't fix the possible flag upgrade issue.
I don't think we need to change pg_upgrade's ControlData controldata;
structure as the information may not be needed there and the while
loop there specifically parses/searches for the required
pg_controldata output texts. Am I missing something here?
> Why are you defining CHECKPOINT_KIND_TEXT_LENGTH twice? You
> should just define it in some sensible header used by both files, or better
> have a new function to take care of that rather than having the code
> duplicated.
Yeah, added the macro in pg_control.h. I also wanted to have a common
function to get checkpoint kind text and place it in
controldata_utils.c, but it doesn't have xlog.h included, so no
checkpoint flags there, hence I refrained from the common function
idea.
I think we don't need to print the checkpoint kind in pg_resetwal.c's
PrintControlValues because the pg_resetwal changes the checkpoint and
PrintControlValues just prints the fields that will not be
reset/changed by pg_resetwal. Am I missing something here?
Attaching v4.
Not related to this patch: by looking at the way the fields (like
"Latest checkpoint's TimeLineID:", "Latest checkpoint's NextOID:"
etc.) of pg_controldata output are being used in pg_resetwal.c,
pg_controldata.c, and pg_upgrade/controldata.c, I'm thinking of having
those fields as macros in pg_control.h
#define PG_CONTROL_LATEST_CHECKPOINT_TLI "Latest checkpoint's TimeLineID:"
#define PG_CONTROL_LATEST_CHECKPOINT_NEXTOID "Latest checkpoint's NextOID:"
and so on for all the pg_controldata fields would be a good idea for
better code manageability and not to miss any field text changes.
If okay, I will discuss this in a separate thread.
Regards,
Bharath Rupireddy.