Re: Multiple pg_waldump --rmgr options

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Multiple pg_waldump --rmgr options
Date: 2021-06-30 20:39:24
Message-ID: df05ede7-a0a3-9d43-98ca-c0bc2c389a5a@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 28/06/2021 13:34, Daniel Gustafsson wrote:
>> On 18 May 2021, at 15:50, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
>> The reason is that if you specify multiple --rmgr options, only the last one takes effect.
>
> That's in line with how options are handled for most binaries, so this will go
> against that. That being said, I don't think thats a problem here really given
> what this tool is and it's intended usecase.

There is some precedent for this with the pg_dump --table option, for
example.

In general, I think it's weird that the latest option wins. If you
specify the same option multiple times, and it's not something like
--rmgr or --table where it makes sense, it's most likely user error.
Printing an error would be nicer than ignoring all but the last
instance. But I'm not going to try changing that now.

>> I propose the attached to allow selecting multiple rmgrs
>
> I agree with the other +1's in this thread, and am marking this as ready for
> committer.
>
> As a tiny nitpick for readability, I would move this line inside the string
> comparison case where the rmgr is selected. Not that it makes any difference
> in practice, but since that's where the filtering is set it seems a hair
> tidier.
> + config.filter_by_rmgr_enabled = true;

Ok, changed it that way.

I tried to be defensive against WAL records with bogus xl_rmid values here:

> @@ -1098,8 +1100,9 @@ main(int argc, char **argv)
> }
>
> /* apply all specified filters */
> - if (config.filter_by_rmgr != -1 &&
> - config.filter_by_rmgr != record->xl_rmid)
> + if (config.filter_by_rmgr_enabled &&
> + (record->xl_rmid < 0 || record->xl_rmid > RM_MAX_ID ||
> + !config.filter_by_rmgr[record->xl_rmid]))
> continue;

But looking closer, that's pointless. We use record->xl_rmid directly as
array index elsewhere, and that's OK because ValidXLogRecordHeader()
checks that xl_rmid <= RM_MAX_ID. And the 'xl_rmid < 0' check is
unnecessary because the field is unsigned. So I'll remove those, and
commit this tomorrow.

- Heikki

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-06-30 20:43:58 New committers: Daniel Gustafsson and John Naylor
Previous Message Tom Lane 2021-06-30 20:15:14 Re: Preventing abort() and exit() calls in libpq