Date: Fri, 9 Apr 2021 20:04:11 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Michael Paquier <michael@paquier.xyz>
cc: Julien Rouhaud <rjuju123@gmail.com>,
    "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
    "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: psql - add SHOW_ALL_RESULTS option
In-Reply-To: <YHAxrQPk2E62wQSS@paquier.xyz>
Message-ID: <alpine.DEB.2.22.394.2104091947240.3439700@pseudo>
References: <1ec5ef49-b5c8-4129-e680-fe42f8d0670b@2ndquadrant.com>
 <alpine.DEB.2.22.394.2104062323150.2555247@pseudo>
 <OSZPR01MB631017521EE6887ADC9492E8FD759@OSZPR01MB6310.jpnprd01.prod.outlook.com>
 <alpine.DEB.2.22.394.2104071516470.2724014@pseudo>
 <OSZPR01MB6310325023E8B4109BB2AB90FD749@OSZPR01MB6310.jpnprd01.prod.outlook.com>
 <20210408163050.fca7dwb37zszbsmm@nol>
 <alpine.DEB.2.22.394.2104081858500.3192110@pseudo>
 <20210408171135.ekhdf5nvqss554q3@nol> <YG+okFpc3JaKN/zB@paquier.xyz>
 <alpine.DEB.2.22.394.2104090828330.3253600@pseudo>
 <YHAxrQPk2E62wQSS@paquier.xyz>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="2056645051-274101562-1617991451=:3439700"
Archived-At: 
 <https://www.postgresql.org/message-id/alpine.DEB.2.22.394.2104091947240.3439700%40pseudo>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--2056645051-274101562-1617991451=:3439700
Content-Type: text/plain; format=flowed; charset=UTF-8
Content-Transfer-Encoding: 8BIT


>> Yep, it looks much better. I found it strange that the later did a reset but
>> was not doing the set.
>>
>> Attached v2 does as you suggest.
>
> Close enough.  I was thinking about this position of the attached,
> which is more consistent with the rest.

Given the structural complexity of the function, the end of the file 
seemed like a good place to have an all-path-guaranteed reset.

I find it a little bit strange to have the Set at the upper level and the 
Reset in many… but not all branches, though.

For instance the on_error_rollback_savepoint/svptcmd branch includes a 
reset long after many other conditional resets, I cannot guess whether the 
initial set is still active or has been long wiped out and this query is 
just not cancellable.

Also, ISTM that in the worst case a cancellation request is sent to a 
server which is idle, in which case it will be ignored, so the code should 
be in no hurry to clean it, at least not at the price of code clarity.

Anyway, the place you suggest seems ok.

-- 
Fabien.
--2056645051-274101562-1617991451=:3439700--