Re: Proposal : For Auto-Prewarm.

On Thu, Jul 6, 2017 at 10:52 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> 3.
>> -- 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.

+ if (buf_state & BM_TAG_VALID &&
+ ((buf_state & BM_PERMANENT) || dump_unlogged))
I have changed it now the final call to dump_now during shutdown or if
called through autoprewarm_dump_now() only we dump blockinfo of
unlogged tables.

>>
>> -- 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 c
> (procsignal_sigusr1_handler).  Have you verified that it will never
> set the InterruptPending flag?

Okay now CHECK_FOR_INTERRUPTS is called for both.

>
>>>
>>> 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.

my intention is to unlink when ever possible and when ever control is
within the function. I thought it is okay if we error inside called
function. If temp file is left there that will not be a problem as it
will be reused(truncated first) for next dump. If you think it is
needed I will add a try() catch() around, to catch any error and then
remove the file.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com