Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions

On 8/4/19 4:13 AM, Tom Lane wrote:
> Ian Barwick <ian(dot)barwick(at)2ndquadrant(dot)com> writes:
>> On 8/3/19 7:27 AM, Tom Lane wrote:
>>> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
>>>> The main issue however is that no code was written yet.
>
>>> Seems like it ought to be relatively simple ... but I didn't look.
>
>> The patch I originally sent does exactly this.
>
> Ah, you did send a patch, but that tries to maintain the existing behavior
> of replacing the last occurrence in-place. I think it's simpler and more
> sensible to just make a sweep to delete all matches, and then append the
> new setting (if any) at the end, as attached.

Yes, that is less convoluted.

> A more aggressive patch would try to de-duplicate the entire list, not
> just the current target entry ... but I'm not really excited about doing
> that in a back-patchable bug fix.

I thought about doing that but it's more of a nice-to-have and not essential
to fix the issue, as any other duplicate entries will get removed the next
time ALTER SYSTEM is run on the entry in question. Maybe as part of a future
improvement.

> I looked at the TAP test you proposed and couldn't quite convince myself
> that it was worth the trouble. A new test within an existing suite
> would likely be fine, but a whole new src/test/ subdirectory just for
> pg.auto.conf seems a bit much. (Note that the buildfarm and possibly
> the MSVC scripts would have to be taught about each such subdirectory.)

Didn't know that, but couldn't find anywhere obvious to put the test.

> At the same time, we lack any better place to put such a test :-(.
> Maybe it's time for a "miscellaneous TAP tests" subdirectory?

Sounds reasonable.

Regards

Ian Barwick

--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services