Re: Proposal : For Auto-Prewarm.

On Sun, Jul 2, 2017 at 10:32 PM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
> On Tue, Jun 27, 2017 at 11:41 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
>> On Fri, Jun 23, 2017 at 5:45 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>>>
>>> Also, I find it a bit messy that launch_autoprewarm_dump() doesn't
>>> detect an autoprewarm process already running. I'd want this to
>>> return NULL or an error if called for a 2nd time.
>>
>> We log instead of error as we try to check only after launching the
>> worker and inside worker. One solution could be as similar to
>> autoprewam_dump_now(), the autoprewarm_start_worker() can init shared
>> memory and check if we can launch worker in backend itself. I will try
>> to fix same.
>
> I have fixed it now as follows

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.

2.
+autoprewarm_dump_now(PG_FUNCTION_ARGS)
+{
+ uint32 num_blocks = 0;
+
..
+ PG_RETURN_INT64(num_blocks);
..
}

Is there any reason for using PG_RETURN_INT64 instead of PG_RETURN_UINT32?

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.

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?

5.
+ * Each entry of BlockRecordInfo consists of database, tablespace, filenode,
+ * forknum, blocknum. Note that this is in the text form so that the dump
+ * information is readable and can be edited, if required.
+ */

In the above comment, you are saying that the dump file is in text
form whereas in the code you are using binary form. I think code
should match comments. Is there a reason of preferring binary over
text or vice versa?

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?

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?

8.
+ file = AllocateFile(transient_dump_file_path, PG_BINARY_W);
+ if (!file)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m",
+ transient_dump_file_path)));
+
+ ret = fprintf(file, "<<%u>>\n", num_blocks);
+ if (ret < 0)
+ {
+ int save_errno = errno;
+
+ FreeFile(file);

I think you don't need to close the file in case of error, it will be
automatically taken care in case of error (via transaction abort
path).

9.
+ /* Register autoprewarm load. */
+ setup_autoprewarm(&autoprewarm_worker, "autoprewarm", "autoprewarm_main",
+ Int32GetDatum(TASK_PREWARM_BUFFERPOOL), 0, 0);

What does "load" signify in above comment? Do you want to say worker instead?

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