Re: parallel workers and client encoding

I'm happy with this patch.


On 6/29/16 12:41 PM, Robert Haas wrote:
> On Tue, Jun 28, 2016 at 10:10 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 6/27/16 5:37 PM, Robert Haas wrote:
>>> Please find attached an a patch for a proposed alternative approach.
>>> This does the following:
>>>
>>> 1. When the client_encoding GUC is changed in the worker,
>>> SetClientEncoding() is not called.
>>
>> I think this could be a problem, because then the client encoding in the
>> background worker process is inherited from the postmaster, which could in
>> theory be anything.  I think you need to set it at least once to the correct
>> value.
>
> Fixed in the attached version.
>
>>> Thus, GetClientEncoding() in the
>>> worker always returns the database encoding, regardless of how the
>>> client encoding is set.  This is better than your approach of calling
>>> SetClientEncoding() during worker startup, I think, because the worker
>>> could call a parallel-safe function with SET clause, and one of the
>>> GUCs temporarily set could be client_encoding.  That would be stupid,
>>> but somebody could do it.
>>
>> I think if we're worried about this, then this should be an error, but
>> that's a minor concern.
>
> We can't actually throw an error at that point, because we really do
> want the GUC to have the same value in the worker as it does in the
> leader.  Otherwise, anything that can observe the value of an
> arbitrary GUC suddenly becomes parallel-restricted, which is certainly
> unwanted.
>
>> I realize that we don't have a good mechanism in the GUC code to distinguish
>> these two situations.
>>
>> Then again, this shouldn't be so much different in concept from the
>> restoring of GUC variables in the EXEC_BACKEND case.  There is special code
>> in set_config_option() to bypass some of the rules in that case.
>> RestoreGUCState() should be able to get the same sort of pass.
>
> I think we can use the existing flag InitializingParallelWorker to
> handle this case.  See attached.
>
>> Also, set_config_option() knows something about what settings are allowed in
>> a parallel worker, so I wonder if setting client_encoding would even work in
>> spite of that?
>
> I think that with this change, a SET client_encoding on a supposedly
> parallel-safe function will just fail to have any effect when the
> function runs inside a worker.  The attached patch will make that kind
> of thing fail outright when attempted inside a parallel worker.
>
>>> 2. A new function pq_getmsgrawstring() is added.  This is like
>>> pq_getmsgstring() but it does no encoding conversion.
>>> pq_parse_errornotice() is changed to use pq_getmsgrawstring() instead
>>> of pq_getmsgstring().  Because of (1), when the leader receives an
>>> ErrorResponse or NoticeResponse from the worker, it will not have been
>>> subject to encoding conversion; because of this item, the leader will
>>> not try to convert it either when initially parsing it.  So the extra
>>> encoding round-trip is avoided.
>>
>> I like that.
>>
>>> 3. The changes for NotifyResponse which you proposed are included
>>> here, but with the modification that pq_getmsgrawstring() is used.
>>
>> and that.
>
> Thanks for the review.
>
>
>
>


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services