Re: Add information to rm_redo_error_callback()

Hi,

Thanks for the feedback.

On 8/11/20 12:03 PM, Masahiko Sawada wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, 11 Aug 2020 at 15:30, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> On Tue, Aug 11, 2020 at 02:45:50PM +0900, Masahiko Sawada wrote:
>>> Thank you for updating the patch!
>>>
>>> The patch looks good to me. I've set this patch as Ready for Committer.
>> + for (block_id = 0; block_id <= record->max_block_id; block_id++)
>> + {
>> + RelFileNode rnode;
>> + ForkNumber forknum;
>> + BlockNumber blknum;
>>
>> Doesn't this potentially create duplicate information in some of the
>> RM's desc() callbacks, and are we sure that the information provided
>> is worth having for all the RMs? As one example, gin_desc() looks at
>> some of the block information, so there are overlaps.
> Yeah, there is duplicate information in some RMs. I thought that we
> can change individual RM’s desc() functions to output the block
> information but as long as I see the pg_waldump outputs these are not
> annoying to me and many of RM’s desc() doesn’t show the block
> information.

Having this "pg_waldump" kind of format in this place
(rm_redo_error_callback()) ensures that we'll always see the same piece
of information during rm_redo.

I think it's good to guarantee that we'll always see the same piece of
information (should a new RM desc() be created in the future for
example), even if it could lead to some information overlap in some cases.

>> It may be
>> worth thinking about showing more information for has_image and
>> apply_image if a block is in_use?
> Yes. I’m okay with adding information for has_image and apply_image
> but IMHO I'm not sure how these shown in errcontext would help. If an
> error related to has_image or apply_image happens, errmsg should show
> something detailed information about FPI.

I am ok too, but I am also not sure that errcontext is the right place
for that.

Thanks

Bertrand

>
> Regards,
>
> --
> Masahiko Sawada http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services