Thread: [PATCH] New [relation] option engine
Hi! I'd like to introduce a patch that reworks options processing. This patch detaches code for options processing and validating from the code that uses these options. This will allow to reuse same code in any part of postgres that may need options. Currently there are three ways of defining options, and this code is scattered along the source tree, that leads to a problems: code is hard to understand, hard to maintain, I saw several options added or changed, and and in one of two cases, there was one or another mistake done because of code unclearity. General idea: There is an object OptionSpec that describes how single option should be parsed, validated, stored into bytea, etc. OptionSpecSet is a list of OptionSpecs, that describes all possible options for certain object (e.g. options for nbtree index relation) All options related functions that are not depended on context were moved to src/backend/access/common/options.c. These functions receives OptionSpecSet (or OptionSpec) and option data that should be processed. And OptionSpecSet should contain all information that is needed for proper processing. Options from the perspective of the option engine can exist in four representation: - defList - the way they came from SQL parser - TEXT[] - the way they are stored in pg_class or similar place - Bytea - the way they stored in C-structure, for caching and using in postgres code that uses these options - Value List - is an internal representation that is used while parsing, validating and converting between representations See code comments for more info. There are functions that is used for conversion from one representation to another: - optionsDefListToRawValues : defList -> Values - optionsTextArrayToDefList :TEXT[] -> defList - optionsTextArrayToRawValues : TEXT[] -> Values - optionsValuesToTextArray: Values -> TEXT[] - optionsValuesToBytea: Values -> Bytea This functions are called from meta-functions that is used when postgres receives an SQL command for creating or updating options: - optionsDefListToTextArray - when options are created - optionsUpdateTexArrayWithDefList - when option are updated. They also trigger validation while processing. There are also functions for SpecSet processing: - allocateOptionsSpecSet - optionsSpecSetAddBool - optionsSpecSetAddBool - optionsSpecSetAddReal - optionsSpecSetAddEnum - optionsSpecSetAddString For index access methods "amoptions" member function that preformed option processing, were replaced with "amreloptspecset" member function that provided an SpecSet for reloptions for this AM, so caller can trigger option processing himself. For other relation types options have been processing by "fixed" functions, so these processing were replaced with "fixed" SpecSet providers + processing using that SpecSet. Later on these should be moved to access method the same way it is done in indexes. I plan to do it after this patch is commit. As for local options, that is used of opclass options, I kept all current API, but made this API a wrapper around new option engine. Local options should be moved to unified option engine API later on. I hope to do it too. This patch does not change any of postgres behaviour (even if this behaviour is not quite right). The only change is text of the warning for unexisting option in toast namespace. But I hope this change is for better. The only problem I am not sure how to solve is an obtaining a LockMode. To get a LockMode for option , you need a SpecSet. For indexes we get SpecSets via AccessMethod. To get access for AccessMethod, you need relation C- structure. To get relation structure you need open relation with NoLock mode. But if I do it, I get Assert in relation_open. (There were no such Assert when I started this work, it appeared later). All code related to this issue is marked with FIXME comments. I've commented that Assert out, but not quite sure I was right. Here I need a help of more experienced members of community. This quite a big patch. Unfortunately it can't be split into smaller parts. I also suggest to consider this patch as an implementation of general idea, that works well. I have ideas in mind to make it better, but it will be infinite improvement process that will never lead to final commit. So if the concept is good and implementation is good enough, I suggest to commit it, and make it better later on by smaller patches over it. If it is not good enough, let me know, I will try to make it good. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
Hi, On 2022-02-14 00:43:36 +0300, Nikolay Shaplov wrote: > I'd like to introduce a patch that reworks options processing. This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3536.log Given that this patch has been submitted just to the last CF and that there's been no action on it, I don't see this going into 15. Therefore I'd like to move it to the next CF? Greetings, Andres Freund
В письме от вторник, 22 марта 2022 г. 03:46:10 MSK пользователь Andres Freund написал: > > I'd like to introduce a patch that reworks options processing. > This doesn't apply anymore: http://cfbot.cputube.org/patch_37_3536.log Thank you for sending this notice! I've updated the patch to include new changes, that have been made in postgres recently. > Given that this patch has been submitted just to the last CF and that > there's been no action on it, I don't see this going into 15. Yes it is not to be added in 15 for sure. > Therefore I'd like to move it to the next CF? The only reason it can be kept in current CF, is to keep it in front of the eyes of potential reviewers for one more week. So it is up to you, if you consider it does not worth it, you can move it now. No great harm would be done then. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
A minor fix to get rid of maybe-uninitialized warnings. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от воскресенье, 27 марта 2022 г. 15:19:41 MSK пользователь Nikolay Shaplov написал: > A minor fix to get rid of maybe-uninitialized warnings. Same patch, but properly rebased to current master head. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от суббота, 16 апреля 2022 г. 17:15:34 MSK пользователь Nikolay Shaplov написал: Changed NoLock to AccessShareLock. This will remove last FIXME from the patch. As more experienced pg-developers told me, you can call relation_open with AccessShareLock, on a table you are going to work with, any time you like, without being ashamed about it. But this lock is visible when you have prepared transaction, so have to update twophase test of test_decoding extension. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от среда, 4 мая 2022 г. 18:15:51 MSK пользователь Nikolay Shaplov написал: Rebased patch, so it can be applied to current master again. Added static keyword to enum option items definition as it have been done in 09cd33f4 -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
I'm sorry if you've already said this elsewhere, but can you please state what is the *intention* of this patchset? If it's a pure refactoring (but I don't think it is), then it's a net loss, because after pgindent it summarizes as: 58 files changed, 2714 insertions(+), 2368 deletions(-) so we end up 500+ lines worse than the initial story. However, I suspect that's not the final situation, since I saw a comment somewhere that opclass options have to be rewritten to modify this mechanism, and I suspect that will remove a few lines. And you maybe have a more ambitious goal. But what is it? Please pgindent your code for the next submission, making sure to add your new typedef(s) to typedefs.list so that it doesn't generate stupid spaces. After pgindenting you'll notice the argument lists of some functions look bad (cf. commit c4f113e8fef9). Please fix that too. I notice that you kept the commentary about lock levels in the place where they were previously defined. This is not good. Please move each explanation next to the place where each option is defined. For next time, please use "git format-patch" for submission, and write a tentative commit message. The committer may or may not use your proposed commit message, but with it they will know what you're trying to achieve. The translatability marker for detailmsg for enums is wrong AFAICT. You need gettext_noop() around the strings themselves IIRC. I think you need to get rid of the _() call around the variable that receives that value and use errdetail() instead of errdetail_internal(), to avoid double-translating it; but I'm not 100% sure. Please experiment with "make update-po" until you get the messages in the .po file. You don't need braces around single-statement blocks. Thanks -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "World domination is proceeding according to plan" (Andrew Morton)
В письме от воскресенье, 15 мая 2022 г. 15:25:47 MSK пользователь Alvaro Herrera написал: > I'm sorry if you've already said this elsewhere, but can you please > state what is the *intention* of this patchset? If it's a pure > refactoring (but I don't think it is), then it's a net loss, because > after pgindent it summarizes as: > > 58 files changed, 2714 insertions(+), 2368 deletions(-) > > so we end up 500+ lines worse than the initial story. However, I > suspect that's not the final situation, since I saw a comment somewhere > that opclass options have to be rewritten to modify this mechanism, and > I suspect that will remove a few lines. And you maybe have a more > ambitious goal. But what is it? My initial goal was to make options code reusable for any types of options (not only reloptions). While working with this goal I came to conclusion that I have to create completely new option engine that will be used anywhere options (name=value) is needed. This will solve following problems: - provide unified API for options definition. Currently postgres have core-AM options, contrib-AM options and local options for opclasses, each have their own way to define options. This patch will allow to use one API for them all (for opclasses it is still WIP) - Currently core-AM options are partly defined in reloptions.c and partly in AM code. This is error prone. This patch fixes that. - For indexes option definition is moved into AM code, where they should be. For heap it should be moved into AM code later. - There is no difference for core-AM indexes, and contrib-AM indexes options. They use same API. I also tried to write detailed commit message as you've suggested. There my goals is described in more detailed way. > Please pgindent your code for the next submission, making sure to add > your new typedef(s) to typedefs.list so that it doesn't generate stupid > spaces. After pgindenting you'll notice the argument lists of some > functions look bad (cf. commit c4f113e8fef9). Please fix that too. I've tried to pgindent. Hope I did it well. I've manually edited all code lines (not string consts) that were longer then 80 characters, afterwards. Hope it was right decision > I notice that you kept the commentary about lock levels in the place > where they were previously defined. This is not good. Please move each > explanation next to the place where each option is defined. You are right. Tried to find better place for it. I also noticed that I've missed updating initial comment for reloptions.c. Will update it this week, meanwhile will send a patch version without changing that comment, in order not to slow anything down. > For next time, please use "git format-patch" for submission, and write a > tentative commit message. The committer may or may not use your > proposed commit message, but with it they will know what you're trying > to achieve. Done. > The translatability marker for detailmsg for enums is wrong AFAICT. You > need gettext_noop() around the strings themselves IIRC. I think you > need to get rid of the _() call around the variable that receives that > value and use errdetail() instead of errdetail_internal(), to avoid > double-translating it; but I'm not 100% sure. Please experiment with > "make update-po" until you get the messages in the .po file. That part of code was not written by me. It was added while enum options were commit. Then I've just copied it to this patch. I do not quite understand how does it works. But I can say that update-po works well for enum detailmsg, and we actually have gettext_noop(), but it is used while calling optionsSpecSetAddEnum, not when error message is actually printed. But I guess it do the trick. > You don't need braces around single-statement blocks. Tried to remove all I've found. > Thanks Thank you for answering. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
forbid_realloc is only tested in an assert. There needs to be an "if" test for it somewhere (suppose some extension author uses this API and only runs it in assert-disabled environment; they'll never know they made a mistake). But do we really need this option? Why do we need a hardcoded limit in the number of options? In allocateOptionsSpecSet there's a new error message with a typo "grater" which should be "greater". But I think the message is confusingly worded. Maybe a better wording is "the value of parameter XXX may not be greater than YYY". -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
В письме от среда, 18 мая 2022 г. 11:10:08 MSK пользователь Alvaro Herrera написал: > forbid_realloc is only tested in an assert. There needs to be an "if" > test for it somewhere (suppose some extension author uses this API and > only runs it in assert-disabled environment; they'll never know they > made a mistake). But do we really need this option? Why do we need a > hardcoded limit in the number of options? General idea was that it is better to allocate as many option_spec items as we are going to use. It is optional, so if you do not want to allocate exact number of options, then realloc will be used, when we adding one more item, and all allocated items are used. But when you explicitly specify number of items, it is better not to forget to ++ it when you add extra option in the code. That was the purpose of forbid_realloc: to remind. It worked well for, while working with the patch several options were added in the upstream, and this assert reminded me that I should also allocate extra item. If it is run in production without asserts, it is also no big deal, we will just have another realloc. But you are right, variable name forbid_realloc is misleading. It does not really forbid anything, so I changed it to assert_on_realloc, so the name tells what this flag really do. > In allocateOptionsSpecSet there's a new error message with a typo > "grater" which should be "greater". But I think the message is > confusingly worded. Maybe a better wording is "the value of parameter > XXX may not be greater than YYY". This error message is really from bloom index. And here I was not as careful and watchful as I should, because this error message is from the check that was not there in original code. And this patch should not change behaviour at all. So I removed this check completely, and will submit it later. My original patch has a bunch of changes like that. I've removed them all, but missed one in the contrib... :-( Thank you for pointing to it. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
On Mon, 2022-02-14 at 00:43 +0300, Nikolay Shaplov wrote:
> For index access methods "amoptions" member function that preformed
> option
> processing, were replaced with "amreloptspecset" member function that
> provided
> an SpecSet for reloptions for this AM, so caller can trigger option
> processing
> himself.
What about table access methods? There have been a couple attempts to
allow custom reloptions for table AMs. Does this patch help that use
case?
Regards,
Jeff Davis
В письме от понедельник, 11 июля 2022 г. 23:03:55 MSK пользователь Jeff Davis написал: > > For index access methods "amoptions" member function that preformed > > option > > processing, were replaced with "amreloptspecset" member function that > > provided > > an SpecSet for reloptions for this AM, so caller can trigger option > > processing > > himself. > > What about table access methods? There have been a couple attempts to > allow custom reloptions for table AMs. Does this patch help that use > case? This patch does not add custom reloptions for table AM. It is already huge enough, with no extra functionality. But new option engine will make adding custom options for table AM more easy task, as main goal of this patch is to simplify adding options everywhere they needed. And yes, adding custom table AM options is one of my next goals, as soon as this patch is commit. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от вторник, 12 июля 2022 г. 07:30:40 MSK пользователь Nikolay Shaplov написал: > > What about table access methods? There have been a couple attempts to > > allow custom reloptions for table AMs. Does this patch help that use > > case? > > This patch does not add custom reloptions for table AM. It is already huge > enough, with no extra functionality. But new option engine will make adding > custom options for table AM more easy task, as main goal of this patch is to > simplify adding options everywhere they needed. And yes, adding custom > table AM options is one of my next goals, as soon as this patch is commit. And here goes rebased version of the patch, that can be applied to current master. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
2022年7月12日(火) 13:47 Nikolay Shaplov <dhyan@nataraj.su>: > > В письме от вторник, 12 июля 2022 г. 07:30:40 MSK пользователь Nikolay Shaplov > написал: > > > What about table access methods? There have been a couple attempts to > > > allow custom reloptions for table AMs. Does this patch help that use > > > case? > > > > This patch does not add custom reloptions for table AM. It is already huge > > enough, with no extra functionality. But new option engine will make adding > > custom options for table AM more easy task, as main goal of this patch is to > > simplify adding options everywhere they needed. And yes, adding custom > > table AM options is one of my next goals, as soon as this patch is commit. > > And here goes rebased version of the patch, that can be applied to current > master. Hi cfbot reports the patch no longer applies. As CommitFest 2022-11 is currently underway, this would be an excellent time to update the patch. Thanks Ian Barwick
В письме от пятница, 4 ноября 2022 г. 03:47:09 MSK пользователь Ian Lawrence Barwick написал: > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > currently underway, this would be an excellent time to update the patch. Oups! I should have done it before... Fixed -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от пятница, 4 ноября 2022 г. 21:06:38 MSK пользователь Nikolay Shaplov написал: > В письме от пятница, 4 ноября 2022 г. 03:47:09 MSK пользователь Ian Lawrence > Barwick написал: > > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > > currently underway, this would be an excellent time to update the patch. > > Oups! I should have done it before... > Fixed Trying to fix meson build -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от пятница, 4 ноября 2022 г. 22:30:06 MSK пользователь Nikolay Shaplov написал: > > > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > > > currently underway, this would be an excellent time to update the patch. > > > > Oups! I should have done it before... > > Fixed > > Trying to fix meson build Trying to fix compiler warnings. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay Shaplov написал: > > > > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > > > > currently underway, this would be an excellent time to update the > > > > patch. > > > > > > Oups! I should have done it before... > > > Fixed > > > > Trying to fix meson build > > Trying to fix compiler warnings. Patched rebased. Imported changes from 4f981df8 commit (Report a more useful error for reloptions on a partitioned table) -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
On Sun, 20 Nov 2022 at 11:42, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay > Shaplov написал: > > > > > > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > > > > > currently underway, this would be an excellent time to update the > > > > > patch. > > > > > > > > Oups! I should have done it before... > > > > Fixed > > > > > > Trying to fix meson build > > > > Trying to fix compiler warnings. > > Patched rebased. Imported changes from 4f981df8 commit (Report a more useful > error for reloptions on a partitioned table) The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID 92957ed98c5c565362ce665266132a7f08f6b0c0 === === applying patch ./new_options_take_two_v03f.patch patching file src/include/access/reloptions.h Hunk #1 FAILED at 1. 1 out of 4 hunks FAILED -- saving rejects to file src/include/access/reloptions.h.rej [1] - http://cfbot.cputube.org/patch_41_3536.log Regards, Vignesh
On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > > On Sun, 20 Nov 2022 at 11:42, Nikolay Shaplov <dhyan@nataraj.su> wrote: > > > > В письме от воскресенье, 6 ноября 2022 г. 19:22:09 MSK пользователь Nikolay > > Shaplov написал: > > > > > > > > cfbot reports the patch no longer applies. As CommitFest 2022-11 is > > > > > > currently underway, this would be an excellent time to update the > > > > > > patch. > > > > > > > > > > Oups! I should have done it before... > > > > > Fixed > > > > > > > > Trying to fix meson build > > > > > > Trying to fix compiler warnings. > > > > Patched rebased. Imported changes from 4f981df8 commit (Report a more useful > > error for reloptions on a partitioned table) > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > === Applying patches on top of PostgreSQL commit ID > 92957ed98c5c565362ce665266132a7f08f6b0c0 === > === applying patch ./new_options_take_two_v03f.patch > patching file src/include/access/reloptions.h > Hunk #1 FAILED at 1. > 1 out of 4 hunks FAILED -- saving rejects to file > src/include/access/reloptions.h.rej There has been no updates on this thread for some time, so this has been switched as Returned with Feedback. Feel free to open it in the next commitfest if you plan to continue on this. Regards, Vignesh
On 2023-Jan-31, vignesh C wrote: > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > > === Applying patches on top of PostgreSQL commit ID > > 92957ed98c5c565362ce665266132a7f08f6b0c0 === > > === applying patch ./new_options_take_two_v03f.patch > > patching file src/include/access/reloptions.h > > Hunk #1 FAILED at 1. > > 1 out of 4 hunks FAILED -- saving rejects to file > > src/include/access/reloptions.h.rej > > There has been no updates on this thread for some time, so this has > been switched as Returned with Feedback. Feel free to open it in the > next commitfest if you plan to continue on this. Well, no feedback has been given, so I'm not sure this is a great outcome. In the interest of keeping it alive, I've rebased it. It turns out that the only conflict is with the 2022 -> 2023 copyright line update. I have not reviewed it. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "El que vive para el futuro es un iluso, y el que vive para el pasado, un imbécil" (Luis Adler, "Los tripulantes de la noche")
Attachment
В письме от среда, 1 февраля 2023 г. 12:04:26 MSK пользователь Alvaro Herrera написал: > On 2023-Jan-31, vignesh C wrote: > > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > > > The patch does not apply on top of HEAD as in [1], please post a rebased > > > patch: === Applying patches on top of PostgreSQL commit ID > > > 92957ed98c5c565362ce665266132a7f08f6b0c0 === > > > === applying patch ./new_options_take_two_v03f.patch > > > patching file src/include/access/reloptions.h > > > Hunk #1 FAILED at 1. > > > 1 out of 4 hunks FAILED -- saving rejects to file > > > src/include/access/reloptions.h.rej > > > > There has been no updates on this thread for some time, so this has > > been switched as Returned with Feedback. Feel free to open it in the > > next commitfest if you plan to continue on this. > > Well, no feedback has been given, so I'm not sure this is a great > outcome. In the interest of keeping it alive, I've rebased it. It > turns out that the only conflict is with the 2022 -> 2023 copyright line > update. > > I have not reviewed it. Guys sorry, I am totally unable to do just anything this month... Do not have spare resources to switch my attention to anything even this simple... Hope, next month will be better... And I will try to do some more reviewing of other patches... -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Jan-31, vignesh C wrote: > > > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > > > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > > > === Applying patches on top of PostgreSQL commit ID > > > 92957ed98c5c565362ce665266132a7f08f6b0c0 === > > > === applying patch ./new_options_take_two_v03f.patch > > > patching file src/include/access/reloptions.h > > > Hunk #1 FAILED at 1. > > > 1 out of 4 hunks FAILED -- saving rejects to file > > > src/include/access/reloptions.h.rej > > > > There has been no updates on this thread for some time, so this has > > been switched as Returned with Feedback. Feel free to open it in the > > next commitfest if you plan to continue on this. > > Well, no feedback has been given, so I'm not sure this is a great > outcome. In the interest of keeping it alive, I've rebased it. It > turns out that the only conflict is with the 2022 -> 2023 copyright line > update. > > I have not reviewed it. Since there was no response to rebase the patch, I was not sure if the author was planning to continue. Anyways Nikolay Shaplov plans to work on this soon, So I have added this to the next commitfest at [1]. [1] - https://commitfest.postgresql.org/41/3536/ Regards, Vignesh
On 2023-Feb-02, vignesh C wrote: > On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Well, no feedback has been given, so I'm not sure this is a great > > outcome. In the interest of keeping it alive, I've rebased it. It > > turns out that the only conflict is with the 2022 -> 2023 copyright line > > update. > > Since there was no response to rebase the patch, I was not sure if the > author was planning to continue. Anyways Nikolay Shaplov plans to work > on this soon, So I have added this to the next commitfest at [1]. Thank you! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "¿Cómo puedes confiar en algo que pagas y que no ves, y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
On Thu, Feb 2, 2023 at 10:03 AM vignesh C <vignesh21@gmail.com> wrote: > > On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2023-Jan-31, vignesh C wrote: > > > > > On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > > > There has been no updates on this thread for some time, so this has > > > been switched as Returned with Feedback. Feel free to open it in the > > > next commitfest if you plan to continue on this. > > > > Well, no feedback has been given, so I'm not sure this is a great > > outcome. In the interest of keeping it alive, I've rebased it. It > > turns out that the only conflict is with the 2022 -> 2023 copyright line > > update. > > > > I have not reviewed it. > > Since there was no response to rebase the patch, I was not sure if the > author was planning to continue. Anyways Nikolay Shaplov plans to work > on this soon, So I have added this to the next commitfest at [1]. > [1] - https://commitfest.postgresql.org/41/3536/ Nine months have passed, and there hasn't been an update since last time it was returned. It doesn't seem to have been productive to resurrect it to the CF based on "someone plans to work on it soon". I'm re-returning it with feedback. Some time ago, there was a proposal to create a new category "lack of interest", but that also seems to have gone nowhere because of...lack of interest. -- John Naylor
11.11.2023 12:33, John Naylor пишет: > On Thu, Feb 2, 2023 at 10:03 AM vignesh C <vignesh21@gmail.com> wrote: >> >> On Wed, 1 Feb 2023 at 14:49, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >>> >>> On 2023-Jan-31, vignesh C wrote: >>> >>>> On Tue, 3 Jan 2023 at 18:38, vignesh C <vignesh21@gmail.com> wrote: > >>>> There has been no updates on this thread for some time, so this has >>>> been switched as Returned with Feedback. Feel free to open it in the >>>> next commitfest if you plan to continue on this. >>> >>> Well, no feedback has been given, so I'm not sure this is a great >>> outcome. In the interest of keeping it alive, I've rebased it. It >>> turns out that the only conflict is with the 2022 -> 2023 copyright line >>> update. >>> >>> I have not reviewed it. >> >> Since there was no response to rebase the patch, I was not sure if the >> author was planning to continue. Anyways Nikolay Shaplov plans to work >> on this soon, So I have added this to the next commitfest at [1]. >> [1] - https://commitfest.postgresql.org/41/3536/ > > Nine months have passed, and there hasn't been an update since last > time it was returned. It doesn't seem to have been productive to > resurrect it to the CF based on "someone plans to work on it soon". > I'm re-returning it with feedback. > > Some time ago, there was a proposal to create a new category "lack of > interest", but that also seems to have gone nowhere because of...lack > of interest. I've rebased patch, so it could be add to commitfest again. ------ Yura Sokolov
Attachment
On Fri, Dec 08, 2023 at 08:10:29AM +0300, Yura Sokolov wrote: > I've rebased patch, so it could be add to commitfest again. This is a 270kB patch with quite a few changes, and a lot of code moved around: > 47 files changed, 2592 insertions(+), 2326 deletions(-) Could it be possible to split that into more successive steps to ease its review? -- Michael
Attachment
В письме от пятница, 8 декабря 2023 г. 08:59:41 MSK пользователь Michael Paquier написал: > > I've rebased patch, so it could be add to commitfest again. > > This is a 270kB patch with quite a few changes, and a lot of code > > moved around: > > 47 files changed, 2592 insertions(+), 2326 deletions(-) > > Could it be possible to split that into more successive steps to ease > its review? Theoretically I can create patch with full options.c as it is in the patch now, and use that code only in index AM, and keep reloption.c mostly unchanged. This will be total mess with two different options mechanisms working in the same time, but this might be much more easy to review. When we are done with the first step, we can change the rest. If this will help to finally include patch into postgres, I can do it. Will that help you to review? -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
On 2023-Dec-08, Nikolay Shaplov wrote: > Theoretically I can create patch with full options.c as it is in the patch > now, and use that code only in index AM, and keep reloption.c mostly > unchanged. > > This will be total mess with two different options mechanisms working in the > same time, but this might be much more easy to review. When we are done with > the first step, we can change the rest. > If this will help to finally include patch into postgres, I can do it. Will > that help you to review? I don't think that's better, because we could create slight inconsistencies between the code used for index AMs and the users of reloptions. I'm not seeing any reasonable way to split this patch in smaller ones. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No necesitamos banderas No reconocemos fronteras" (Jorge González)
В письме от пятница, 8 декабря 2023 г. 15:59:09 MSK пользователь Alvaro Herrera написал: > > Theoretically I can create patch with full options.c as it is in the patch > > now, and use that code only in index AM, and keep reloption.c mostly > > unchanged. > > > > This will be total mess with two different options mechanisms working in > > the same time, but this might be much more easy to review. When we are > > done with the first step, we can change the rest. > > If this will help to finally include patch into postgres, I can do it. > > Will > > that help you to review? > > I don't think that's better, because we could create slight > inconsistencies between the code used for index AMs and the users of > reloptions. I've written quite good regression tests for it, there should be no inconsistency. > I'm not seeing any reasonable way to split this patch in smaller ones. Actually me neither. Not a good one anyway. But if somebody really need it to be split, it can be done that way. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
2024-01 Commitfest. Hi, This patch has a CF status of "Needs Review" [1], but it seems there were CFbot test failures last time it was run [2]. Please have a look and post an updated version if necessary. ====== [1] https://commitfest.postgresql.org/46/4688/ [2] https://cirrus-ci.com/task/5066432363364352 Kind Regards, Peter Smith.
В письме от понедельник, 22 января 2024 г. 08:24:13 MSK пользователь Peter Smith написал: Hi! I've updated the patch, and it should work with modern master branch. > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there were CFbot test failures last time it was run [2]. Please have a > look and post an updated version if necessary. Do we still miss notification facility for CFbot? May be somebody have added it since the time I've checked last time? > > ====== > [1] https://commitfest.postgresql.org/46/4688/ > [2] https://cirrus-ci.com/task/5066432363364352 > > Kind Regards, > Peter Smith. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
В письме от четверг, 1 февраля 2024 г. 09:58:32 MSK пользователь Nikolay Shaplov написал: I' ve updated the patch again, since the output of the src/test/modules/test_oat_hooks test have changed. This test began to register namespace lookups, and since options internals have been changed much, number of lookups have been changed too. So I just fixed the expected file. > Hi! > > I've updated the patch, and it should work with modern master branch. > > > 2024-01 Commitfest. > > > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > > there were CFbot test failures last time it was run [2]. Please have a > > look and post an updated version if necessary. > > Do we still miss notification facility for CFbot? May be somebody have added > it since the time I've checked last time? > > > ====== > > [1] https://commitfest.postgresql.org/46/4688/ > > [2] https://cirrus-ci.com/task/5066432363364352 > > > > Kind Regards, > > Peter Smith. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Attachment
On Wed, Feb 7, 2024 at 2:45 PM Nikolay Shaplov <dhyan@nataraj.su> wrote:
>
> В письме от четверг, 1 февраля 2024 г. 09:58:32 MSK пользователь Nikolay
> Shaplov написал:
>
> I' ve updated the patch again, since the output of the
> src/test/modules/test_oat_hooks test have changed.
> This test began to register namespace lookups, and since options internals
> have been changed much, number of lookups have been changed too. So I just
> fixed the expected file.
>
> > Hi!
> >
> > I've updated the patch, and it should work with modern master branch.
> >
you've changed the `typedef struct IndexAmRoutine`
then we also need to update doc/src/sgml/indexam.sgml.
some places you use "Spec Set", other places you use "spec set"
+typedef struct options_spec_set
+{
+ option_spec_basic **definitions;
+ int num; /* Number of spec_set items in use */
+ int num_allocated; /* Number of spec_set items allocated */
+ bool assert_on_realloc; /* If number of items of the spec_set were
+ * strictly set to certain value assert on
+ * adding more items */
+ bool is_local; /* If true specset is in local memory context */
+ Size struct_size; /* Size of a structure for options in binary
+ * representation */
+ postprocess_bytea_options_function postprocess_fun; /* This function is
+ * called after options
+ * were converted in
+ * Bytea representation.
+ * Can be used for extra
+ * validation etc. */
+ char *namspace; /* spec_set is used for options from this
+ * namespase */
+} options_spec_set;
here the comments, you used "spec_set".
maybe we can make it more consistent?
typedef enum option_value_status
{
OPTION_VALUE_STATUS_EMPTY, /* Option was just initialized */
OPTION_VALUE_STATUS_RAW, /* Option just came from syntax analyzer in
* has name, and raw (unparsed) value */
OPTION_VALUE_STATUS_PARSED, /* Option was parsed and has link to catalog
* entry and proper value */
OPTION_VALUE_STATUS_FOR_RESET, /* This option came from ALTER xxx RESET */
} option_value_status;
I am slightly confused.
say
`create table a(a1 int) with (foo=bar).`
to make table a created successfully:
we need to check if variable foo is valid or not, if not then error
out immediately,
we also need to check if the variable bar is within the expected bound.
overall I am not sure about the usage of option_value_status.
using some real example demo different option_value_status usage would be great.
+ /*
+ * If this option came from RESET statement we should throw error
+ * it it brings us name=value data, as syntax analyzer do not
+ * prevent it
+ */
+ if (def->arg != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("RESET must not include values for parameters")));
+ errmsg("RESET must not include values for parameters")));
the error message seems not so good?
you declared as:
options_spec_set *get_stdrd_relopt_spec_set(bool is_for_toast);
but you used as:
options_spec_set * get_stdrd_relopt_spec_set(bool is_heap)
maybe we refactor the usage as
`options_spec_set * get_stdrd_relopt_spec_set(bool is_for_toast)`
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("Valid values are between \"%d\" and \"%d\".",
+ optint->min, optint->max)));
maybe
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("valid values are between %d and %d.",
+ optint->min, optint->max)));
+ if (validate && (option->values.real_val < optreal->min ||
+ option->values.real_val > optreal->max))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("Valid values are between \"%f\" and \"%f\".",
+ optreal->min, optreal->max)));
maybe
+ if (validate && (option->values.real_val < optreal->min ||
+ option->values.real_val > optreal->max))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value %s out of bounds for option \"%s\"",
+ value, option->gen->name),
+ errdetail("valid values are between %f and %f.",
+ optreal->min, optreal->max)));
I think the above two, change errdetail to errhint would be more appropriate.
I've attached v7, 0001 and 0002.
v7, 0001 is the rebased v6, v7, 0002 is miscellaneous minor cosmetic fix.
Attachment
В письме от понедельник, 3 июня 2024 г. 10:52:02 MSK пользователь jian he
написал:
Hi!
Thank you for giving attention to my patch, and sorry, it takes me some time
to deal with such things.
By the way, what is your further intentions concerning this patch? Should I
add you as a reviewer, or you are just passing by? Or add you as second
author, if you are planning to suggest big changes
So I tried to fix all issues you've pointed out:
> you've changed the `typedef struct IndexAmRoutine`
> then we also need to update doc/src/sgml/indexam.sgml.
Oh. You are right. I've fixed that. The description I've created is quite
short. I do not know what else I can say there without diving deep into
implementation details. Do you have any ideas what can be added there?
> some places you use "Spec Set", other places you use "spec set"
...
> here the comments, you used "spec_set".
> maybe we can make it more consistent?
This sounds reasonable. I've changed it to Spec Set and Options Spec Set
wherever I found alternative spelling.
> typedef enum option_value_status
> {
> OPTION_VALUE_STATUS_EMPTY, /* Option was just initialized */
> OPTION_VALUE_STATUS_RAW, /* Option just came from syntax analyzer in
> * has name, and raw (unparsed) value */
> OPTION_VALUE_STATUS_PARSED, /* Option was parsed and has link to catalog
> * entry and proper value */
> OPTION_VALUE_STATUS_FOR_RESET, /* This option came from ALTER xxx RESET */
> } option_value_status;
> overall I am not sure about the usage of option_value_status.
> using some real example demo different option_value_status usage would be
> great.
I've added explanatory comment before option_value_status enum explaining what
is what. Hope it will make things more clear.
If not me know and we will try to find out what comments should be added
>
> + errmsg("RESET must not include values for parameters")));
> the error message seems not so good?
eh... It does seem to be not good, but I've copied it from original code
https://github.com/postgres/postgres/blob/
00ac25a3c365004821e819653c3307acd3294818/src/backend/access/common/
reloptions.c#L1231
I would not dare changing it. I've already changed to many things here.
> you declared as:
> options_spec_set *get_stdrd_relopt_spec_set(bool is_for_toast);
> but you used as:
> options_spec_set * get_stdrd_relopt_spec_set(bool is_heap)
>
> maybe we refactor the usage as
> `options_spec_set * get_stdrd_relopt_spec_set(bool is_for_toast)`
My bad.
I've changed is_for_toast to is_heap once, and I guess I've missed function
declaration... Fixed it to be consistent.
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("value %s out of bounds for option \"%s\"",
> + value, option->gen->name),
> + errdetail("Valid values are between \"%d\" and \"%d\".",
> + optint->min, optint->max)));
.....
> I think the above two, change errdetail to errhint would be more
> appropriate.
Same as above... I've just copied error messages from the original code.
Fixing them better go as a separate patch I guess...
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachment
While fixing the patch according to jian he recommendations, I come to a conclusion that using void* pointer to a structure is really bad idea. Do not know why I ever did it that way from start. So I fixed it. Changes of this fix can be seen here: https://gitlab.com/dhyannataraj/postgres/-/commit/be4d9e16 The whole patch is attached to this mail. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su