Horton

Thread

Commits

  1. Test that it works to RESET an invalid reloption

  1. [PATCH] minor reloption regression tests improvement

    Nikolay Shaplov <dhyan@nataraj.su> — 2022-02-11T09:51:22Z

    I'd like to suggest a patch for reloption regression tests.

This patch tests case, that can be rarely met in actual life: when reloptions 
have some illegal option set (as a result of malfunction or extension 
downgrade or something), and user tries to remove this option by using RESET.
Current postgres behaviour is to actually remove this option.

Like:

UPDATE pg_class  SET reloptions = '{illegal_option=4}' 
   WHERE oid = 'reloptions_test'::regclass;

ALTER TABLE reloptions_test RESET (illegal_option);

Why this should be tested:
1. It is what postgres actually do now.
2. This behaviour is reasonable. DB User can fix problem without updating 
pg_class, having rights to change his own table.
3. Better to get test alarm, if this behavior is accidentally changed.  

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

  2. Re: [PATCH] minor reloption regression tests improvement

    Greg Stark <stark@mit.edu> — 2022-03-07T17:04:49Z

    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.

  3. Re: [PATCH] minor reloption regression tests improvement

    Nikolay Shaplov <dhyan@nataraj.su> — 2022-03-07T17:50:53Z

    В письме от понедельник, 7 марта 2022 г. 20:04:49 MSK пользователь Greg Stark 
написал:
> 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.

This is a regression test. To test if behaviour brocken or not.

Actually I came to the idea of the test when I wrote my big reloption patch. 
I've broken this behaviour by mistake and  did not notice it as it have not 
been properly tested. So I decided it would be goo to add test to it before 
adding and big patches.

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

  4. Re: [PATCH] minor reloption regression tests improvement

    Jacob Champion <jchampion@timescale.com> — 2022-06-29T21:38:48Z

    This patch is hanging open in the March commitfest. There was a bit of back-and-forth on whether it should be rejected, but no clear statement on the issue, so I'm going to mark it Returned with Feedback. If you still feel strongly about this patch, please feel free to re-register it in the July commitfest.

Thanks,
--Jacob

  5. Re: [PATCH] minor reloption regression tests improvement

    Nikolay Shaplov <dhyan@nataraj.su> — 2022-06-30T03:47:48Z

    В письме от четверг, 30 июня 2022 г. 00:38:48 MSK пользователь Jacob Champion 
написал:
> This patch is hanging open in the March commitfest. There was a bit of
> back-and-forth on whether it should be rejected, but no clear statement on
> the issue, so I'm going to mark it Returned with Feedback. If you still
> feel strongly about this patch, please feel free to re-register it in the
> July commitfest.

Hi! I am surely feel this patch is important. I have bigger patch 
https://commitfest.postgresql.org/38/3536/ and this test makes sense as a part 
of big work of options refactoring,

I am also was strongly advised to commit things chopped into smaller parts, 
when possible. This test can be commit separately so I am doing it.


-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

  6. Re: [PATCH] minor reloption regression tests improvement

    Nikolay Shaplov <dhyan@nataraj.su> — 2022-06-30T04:04:25Z

    В письме от четверг, 30 июня 2022 г. 06:47:48 MSK пользователь Nikolay Shaplov 
написал:
 
> Hi! I am surely feel this patch is important. I have bigger patch
> https://commitfest.postgresql.org/38/3536/ and this test makes sense as a
> part of big work of options refactoring,
> 
> I am also was strongly advised to commit things chopped into smaller parts,
> when possible. This test can be commit separately so I am doing it.

Let me again explain why this test is importaint, so potential reviewers can 
easily find this information.

Tests are for developers. You change the code and see that something does not 
work anymore, as it worked before.
When you change the code, you should keep both documented and undocumented 
behaviour. Because user's code can intentionally  or accidentally use it. 

So known undocumented behavior is better to be tested as well as documented 
one. If you as developer want to change undocumented behavior, it is better to 
do it consciously. This test insures that.

I, personally hit this problem while working with reloption code. Older 
version of my new patch changed this behaviour, and I even did not notice 
that. So I decided to write this test to ensure, that nobody ever meet same 
problem. 

And as I said before, I was strongly advised to commit in small parts, when 
possible. This test can be commit separately.  

-- 
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

  7. Re: [PATCH] minor reloption regression tests improvement

    Jacob Champion <jchampion@timescale.com> — 2022-06-30T23:16:24Z

    On Wed, Jun 29, 2022 at 9:04 PM Nikolay Shaplov <dhyan@nataraj.su> wrote:
> В письме от четверг, 30 июня 2022 г. 06:47:48 MSK пользователь Nikolay Shaplov
> написал:
>
> > Hi! I am surely feel this patch is important. I have bigger patch
> > https://commitfest.postgresql.org/38/3536/ and this test makes sense as a
> > part of big work of options refactoring,
> >
> > I am also was strongly advised to commit things chopped into smaller parts,
> > when possible. This test can be commit separately so I am doing it.
>
> Let me again explain why this test is importaint, so potential reviewers can
> easily find this information.
>
> Tests are for developers. You change the code and see that something does not
> work anymore, as it worked before.
> When you change the code, you should keep both documented and undocumented
> behaviour. Because user's code can intentionally  or accidentally use it.

[developer hat] Right, but I think Greg also pointed out the tradeoff
here, and my understanding was that he didn't feel that the tradeoff
was enough. If this is related to a bigger refactoring, it may be
easier to argue the test's value if it's discussed there? (This could
be frustrating if you've just been told to split things up, sorry.)

[CFM hat] Since you feel strongly about the patch, and we're short on
time before the commitfest starts, I have re-registered this. That way
there can be an explicit decision as opposed to a pocket veto by me.

    https://commitfest.postgresql.org/38/3747/

Thanks,
--Jacob

  8. Re: [PATCH] minor reloption regression tests improvement

    Jacob Champion <jchampion@timescale.com> — 2022-08-02T21:26:28Z

    On 6/30/22 16:16, Jacob Champion wrote:
> [CFM hat] Since you feel strongly about the patch, and we're short on
> time before the commitfest starts, I have re-registered this. That way
> there can be an explicit decision as opposed to a pocket veto by me.

[CFM hat] Okay, with another CF come and gone without review I feel much
more confident about closing this as Returned with Feedback.

[dev hat] Specifically I don't think this patch is reviewable alone; it
needs to be grouped with the functionality change that needed the
additional coverage. That way it'll be much easier for a reviewer to
decide whether 1) it's covering the right spots and 2) it's an overall
useful addition.

That doesn't mean you have to smash it into another commit; it can be a
separate test commit as part of a bigger patchset, and the commit
message can include the motivation for why you wrote the new test.

Thanks,
--Jacob

  9. Re: [PATCH] minor reloption regression tests improvement

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-12-08T11:19:22Z

    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í"