On 2022-Mar-07, Greg Stark wrote:
> I don't think this is worth spending time adding tests for. I get what
> you're saying that this is at least semi-intentional behaviour and
> desirable behaviour so it should have tests ensuring that it continues
> to work. But it's not documented behaviour and the test is basically
> testing that the implementation is this specific implementation.
>
> I don't think the test is really a bad idea but neither is it really
> very useful and I think it's not worth having people spend time
> reviewing and discussing. I'm leaning to saying this patch is
> rejected.
I've pushed this patch; I agree with Greg that this is semi-intentional
and desirable, even if not documented. (I doubt we would document it
anyway, as having previously SET an invalid option is not something that
happens commonly; what reason would we have for documenting that it
works to RESET such an option?) As for the implementation being this
specific one, that's something already embedded in all of
reloptions.sql, so I don't really mind that, if we do change it, we'll
have to rewrite pretty much the whole file and not "the whole file
except these three lines".
Lastly, there is a long-running patch proposal to refactor reloptions
quite significantly, so it'll be good to ensure that the new
implementation doesn't break the features we already have; and this one
corner is one thing Nikolay already said his patch had broken and failed
to notice right away.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La verdad no siempre es bonita, pero el hambre de ella sí"
