On 1/18/22 15:41, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>
> I took a quick look at this and agree with the proposed behavior
> change, but also with your self-criticisms:
>
>> We may want to do the same on the server side to make the code blocks
>> look more similar.
>>
>> Also, on the server side the S_ISREG() check gets its own error and that
>> might be a good idea on the client side as well. As it is, the error
>> message on the client is going to be pretty confusing in this case.
>
> Particularly, I think the S_ISREG check should happen before any
> ownership/permissions checks; it just seems saner that way.
I was worried about doing too much refactoring in this commit since I
have hopes and dreams of it being back-patched. But I'll go ahead and do
that and if any part of this can be back-patched we'll consider that
separately.
> The only other nitpick I have is that I'd make the cross-references be
> to the two file names, ie like "Note that similar checks are performed
> in fe-secure-openssl.c ..." References to the specific functions seem
> likely to bit-rot in the face of future code rearrangements.
> I suppose filename references could become obsolete too, but it
> seems less likely.
It's true that functions are more likely to be renamed, but when I
rename a function I then search for all the places where it is used so I
can update them. If the function name appears in a comment that gets
updated as well.
If you would still prefer filenames I have no strong argument against
that, just wanted to explain my logic.
Regards,
-David