Thread: [PATCH] minor reloption regression tests improvement
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
Attachment
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.
В письме от понедельник, 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
This patch is hanging open in the March commitfest. There was a bit of back-and-forth on whether it should be rejected, butno clear statement on the issue, so I'm going to mark it Returned with Feedback. If you still feel strongly about thispatch, please feel free to re-register it in the July commitfest. Thanks, --Jacob
В письме от четверг, 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
Attachment
В письме от четверг, 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
Attachment
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
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
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í"