Re: Proposal : For Auto-Prewarm.

On Wed, Jul 5, 2017 at 6:25 PM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> On Mon, Jul 3, 2017 at 3:34 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> Few comments on the latest patch:
>>
>> 1.
>> + LWLockRelease(&apw_state->lock);
>> + if (!is_bgworker)
>> + ereport(ERROR,
>> + (errmsg("could not perform block dump because dump file is being
>> used by PID %d",
>> + apw_state->pid_using_dumpfile)));
>> + ereport(LOG,
>> + (errmsg("skipping block dump because it is already being performed by PID %d",
>> + apw_state->pid_using_dumpfile)));
>>
>>
>> The above code looks confusing as both the messages are saying the
>> same thing in different words. I think you keep one message (probably
>> the first one) and decide error level based on if this is invoked for
>> bgworker. Also, you can move LWLockRelease after error message,
>> because if there is any error, then it will automatically release all
>> lwlocks.
>
> ERROR is used for autoprewarm_dump_now which is called from the backend.
> LOG is used for bgworker.
> wordings used are to match the context if failing to dump is
> acceptable or not. In the case of bgworker, it is acceptable we are
> not particular about the start time of dump but the only interval
> between the dumps. So if already somebody doing it is acceptable. But
> one who calls autoprewarm_dump_now might be particular about the start
> time of dump so we throw error making him retry same.
>
> The wording's are suggested by Robert(below snapshot) in one of his
> previous comments and I also agree with it. If you think I should
> reconsider this and I am missing something I am open to suggestions.
>

Not an issue, if you and Robert think having two different messages is
better, then let's leave it. One improvement we could do here is to
initialize a boolean global variable for AutoPrewarmWorker, then use
it wherever required.

>> 3.
>> +dump_now(bool is_bgworker)
>> {
>> ..
>> + if (buf_state & BM_TAG_VALID)
>> + {
>> + block_info_array[num_blocks].database = bufHdr->tag.rnode.dbNode;
>> + block_info_array[num_blocks].tablespace = bufHdr->tag.rnode.spcNode;
>> + block_info_array[num_blocks].filenode = bufHdr->tag.rnode.relNode;
>> + block_info_array[num_blocks].forknum = bufHdr->tag.forkNum;
>> + block_info_array[num_blocks].blocknum = bufHdr->tag.blockNum;
>> + ++num_blocks;
>> + }
>> ..
>> }
>>I think there is no use of writing Unlogged buffers unless the dump is
>>for the shutdown. You might want to use BufferIsPermanent to detect the same.
>
> -- I do not think that is true pages of the unlogged table are also
> read into buffers for read-only purpose. So if we miss to dump them
> while we shut down then the previous dump should be used.
>

I am not able to understand what you want to say. Unlogged tables
should be empty in case of crash recovery. Also, we never flush
unlogged buffers except for shutdown checkpoint, refer BufferAlloc and
in particular below comment:

* Make sure BM_PERMANENT is set for buffers that must be written at every
* checkpoint. Unlogged buffers only need to be written at shutdown
* checkpoints, except for their "init" forks, which need to be treated
* just like permanent relations.

>> 4.
>> +static uint32
>> +dump_now(bool is_bgworker)
>> {
>> ..
>> + for (num_blocks = 0, i = 0; i < NBuffers; i++)
>> + {
>> + uint32 buf_state;
>> +
>> + if (!is_bgworker)
>> + CHECK_FOR_INTERRUPTS();
>> ..
>> }
>>
>> Why checking for interrupts is only for non-bgwroker cases?
>
> -- autoprewarm_dump_now is directly called from the backend. In such
> case, we have to handle signals registered for backend in dump_now().
> For bgworker dump_block_info_periodically caller of dump_now() handles
> SIGTERM, SIGUSR1 which we are interested in.
>

Okay, but what about signal handler for SIGUSR1
(procsignal_sigusr1_handler). Have you verified that it will never
set the InterruptPending flag?

>
>> 6.
>> +dump_now(bool is_bgworker)
>> {
>> ..
>> + (void) durable_rename(transient_dump_file_path, AUTOPREWARM_FILE, ERROR);
>> + apw_state->pid_using_dumpfile = InvalidPid;
>> ..
>> }
>>
>> How will pid_using_dumpfile be set to InvalidPid in the case of error
>> for non-bgworker cases?
>
> -- I have a try() - catch() in autoprewarm_dump_now I think that is okay.
>

Okay, then that will work.

>>
>> 7.
>> +dump_now(bool is_bgworker)
>> {
>> ..
>> + (void) durable_rename(transient_dump_file_path, AUTOPREWARM_FILE, ERROR);
>>
>> ..
>> }
>>
>> How will transient_dump_file_path be unlinked in the case of error in
>> durable_rename? I think you need to use PG_TRY..PG_CATCH to ensure
>> same?
>
> -- If durable_rename is failing that seems basic functionality of
> autoperwarm is failing so I want it to be an ERROR. I do not want to
> remove the temp file as we always truncate before reusing it again. So
> I think there is no need to catch all ERROR in dump_now() just to
> remove the temp file.
>

I am not getting your argument here, do you mean to say that if
writing to a transient file is failed then we should remove the
transient file but if the rename is failed then there is no need to
remove it? It sounds strange to me, but maybe you have reason to do
it like that.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com