Re: Get memory contexts of an arbitrary backend process

Hi,

Thanks for all your comments, I updated the patch.


On Tue, Sep 1, 2020 at 12:03 AM Kasahara Tatsuhito 
<kasahara.tatsuhito@gmail.com> wrote:

> - How about managing the status of signal send/receive and dump
> operations on a shared hash or others ?
> Sending and receiving signals, dumping memory information, and
> referencing dump information all work asynchronously.
> Therefore, it would be good to have management information to
> check
> the status of each process.
> A simple idea is that ..
> - send a signal to dump to a PID, it first record following
> information into the shared hash.
> pid (specified pid)
> loc (dump location, currently might be ASAP)
> recv (did the pid process receive a signal? first false)
> dumped (did the pid process dump a mem information?  first
> false)
> - specified process receive the signal, update the status in the
> shared hash, then dumped at specified location.
> - specified process finish dump mem information,  update the
> status
> in the shared hash.

I added a shared hash table consisted of minimal members
mainly for managing whether the file is dumped or not.
Some members like 'loc' seem useful in the future, but I
haven't added them since it's not essential at this point.



On 2020-09-01 10:54, Andres Freund wrote:

>>  CREATE VIEW pg_backend_memory_contexts AS
>> -    SELECT * FROM pg_get_backend_memory_contexts();
>> +    SELECT * FROM pg_get_backend_memory_contexts(-1);
> 
> -1 is odd. Why not use NULL or even 0?

Changed it from -1 to NULL.

>> +             rc = fwrite(&name_len, sizeof(int), 1, fpout);
>> +             rc = fwrite(name, name_len, 1, fpout);
>> +             rc = fwrite(&idlen, sizeof(int), 1, fpout);
>> +             rc = fwrite(clipped_ident, idlen, 1, fpout);
>> +             rc = fwrite(&level, sizeof(int), 1, fpout);
>> +             rc = fwrite(&parent_len, sizeof(int), 1, fpout);
>> +             rc = fwrite(parent, parent_len, 1, fpout);
>> +             (void) rc;                              /* we'll check 
>> for error with ferror */
>> +
>> +     }
> This format is not descriptive. How about serializing to json or
> something? Or at least having field names?

Added field names for each value.

>> +             while (true)
>> +             {
>> +                     CHECK_FOR_INTERRUPTS();
>> +
>> +                     pg_usleep(10000L);
>> +
> 
> Need better signalling back/forth here.

Added a shared hash table that has a flag for managing whether the file 
is dumped or not.
I modified it to use this flag.

>> +     /*
>> +      * Open a temp file to dump the current memory context.
>> +      */
>> +     fpout = AllocateFile(tmpfile, PG_BINARY_W);
>> +     if (fpout == NULL)
>> +     {
>> +             ereport(LOG,
>> +                             (errcode_for_file_access(),
>> +                              errmsg("could not write temporary 
>> memory context file \"%s\": %m",
>> +                                             tmpfile)));
>> +             return;
>> +     }
> 
> Probably should be opened with O_CREAT | O_TRUNC?

AllocateFile() calls fopen() and AFAIU fopen() with mode "w" corresponds 
to open() with "O_WRONLY | O_CREAT | O_TRUNC".

Do you mean I should not use fopen() here?

On 2020-09-24 13:01, Michael Paquier wrote:
> On Thu, Sep 10, 2020 at 09:11:21PM +0900, Kasahara Tatsuhito wrote:
>> I think it's fine to have an interface to delete in an emergency, but
>> I agree that
>> users shouldn't be made aware of the existence or deletion of dump
>> files, basically.
> 
> Per the CF bot, the number of tests needs to be tweaked, because we
> test each entry filtered out with is_deeply(), meaning that the number
> of tests needs to be updated to reflect that if the filtered list is
> changed:
> t/010_pg_basebackup.pl ... 104/109 # Looks like you planned 109 tests
> but ran 110.
> t/010_pg_basebackup.pl ... Dubious, test returned 255 (wstat 65280, 
> 0xff00)
> All 109 subtests passed
> 
> Simple enough to fix.

Incremented the number of tests.


Any thoughts?

Regards,

-- 
Atsushi Torikoshi
NTT DATA CORPORATION