Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

On 2022/02/02 21:59, torikoshia wrote:
>> This may cause users to misunderstand that pg_log_query_plan() fails
>> while the target backend is holding *any* locks? Isn't it better to
>> mention "page-level locks", instead? So how about the following?
>>
>> --------------------------
>> Note that the request to log the query plan will be ignored if it's
>> received during a short period while the target backend is holding a
>> page-level lock.
>> --------------------------
> 
> Agreed.

On second thought, this note is confusing rather than helpful? Because the users don't know when and what operation
needspage-level lock. So now I'm thinking it's better to remove this note.
 


> As you pointed out offlist, the issue could occur even when both pg_log_backend_memory_contexts and pg_log_query_plan
arecalled and it may be better to make another patch.
 

OK.


> You also pointed out offlist that it would be necessary to call LockErrorCleanup() if ProcessLogQueryPlanInterrupt()
canincur ERROR.
 
> AFAICS it can call ereport(ERROR), i.e., palloc0() in NewExplainState(), so I added PG_TRY/CATCH and make it call
LockErrorCleanup()when ERROR occurs.
 

As we discussed off-list, this treatment might not be necessary. Because when ERROR or FATAL error happens,
AbortTransaction()is called and LockErrorCleanup() is also called inside there.
 


> Since I'm not sure how long it will take to discuss this point, the attached patch is based on the current HEAD at
thistime.
 

Thanks for updating the patch!

@@ -5048,6 +5055,12 @@ AbortSubTransaction(void)
       */
      PG_SETMASK(&UnBlockSig);
  
+    /*
+     * When ActiveQueryDesc is referenced after abort, some of its elements
+     * are freed. To avoid accessing them, reset ActiveQueryDesc here.
+     */
+    ActiveQueryDesc = NULL;

AbortSubTransaction() should reset ActiveQueryDesc to save_ActiveQueryDesc that ExecutorRun() set, instead of NULL?
OtherwiseActiveQueryDesc of top-level statement will be unavailable after subtransaction is aborted in the nested
statements.

For example, no plan is logged while the following "select pg_sleep(test())" is running, because the exception inside
test()function aborts the subtransaction and resets ActiveQueryDesc to NULL.
 

create or replace function test () returns int as $$
begin
     perform 1/0;
exception when others then
     return 30;
end;
$$ language plpgsql;

select pg_sleep(test());


-CREATE ROLE regress_log_memory;
+CREATE ROLE regress_log;

Isn't this name too generic? How about regress_log_function, for example?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION