Re: access numeric data in module - Mailing list pgsql-hackers
| From | Michael Paquier |
|---|---|
| Subject | Re: access numeric data in module |
| Date | |
| Msg-id | acNsY2QdJcoKw3kF@paquier.xyz Whole thread Raw |
| In response to | Re: access numeric data in module (Jelte Fennema-Nio <postgres@jeltef.nl>) |
| List | pgsql-hackers |
On Wed, Feb 11, 2026 at 12:24:31AM +0100, Jelte Fennema-Nio wrote: > Yes, it's internally. But to me the whole point of creating an > extension (with native code) is so you can access internal things. > This is the only type that we're currently hiding the internal > structure of, and I don't think numeric is any more special than any > of our other complex data types. This was marked as ready for committer, so I have looked at it to see if it would be possible to salvage something for this release, and my short answer is no, after looking at this thread and the version of the patch posted here: https://www.postgresql.org/message-id/CAJBL5DOz9=QZaXsAgMEt46OdGcWws3hpy3SCBEyqXhHGXqrz1A@mail.gmail.com Now, the longer answer. I agree with Robert's feeling on the matter that it is a bad experience for extension developers to have to copy/paste hundreds if not thousands of in-core code in an extension because we forgot the concept of publishing some in-core APIs that could be used instead. Regarding the patch, it is absolutely unclear to me why this level of copy/paste from numeric.c to numeric.h is the right thing to do, with only one routine rename, this one, so as it is less generic: - * make_result() - + * numeric_make_result() - My opinion is in line with this point from Tom, FWIW: https://www.postgresql.org/message-id/2963166.1740846781@sss.pgh.pa.us We have also that: +extern void alloc_var(NumericVar *var, int ndigits); +extern void free_var(NumericVar *var); +extern void zero_var(NumericVar *var); + +extern void set_var_from_num(Numeric num, NumericVar *dest); +extern void init_var_from_num(Numeric num, NumericVar *dest); +extern void set_var_from_var(const NumericVar *value, NumericVar *dest); Var is a concept that exists in the code, so this is just a blind unthought move of code from one place to the other, and it is not clear at all why and how it would benefit for extensions developers at large. It may benefit for the case of the author, but it seems that we may want to think about the refactoring and the amount of routines to publish at large, around NumericVar. One suggestion that I would like to offer is to implement a test module that has snipets of code one could look at as a start point when working on NumericVar, to demonstrate the benefit of this refactoring. With none of that presented in the patch, it's a bit hard for one to look at these APIs and make sense of how to use NumericVar at all. I feel that it may point at something else, could it make sense to split numeric.c/h and move some of NumericVar into a new set of files? Another thing that would give bonus points for your case is test coverage: do we have corner cases related to NumericVar that are not stressed now but could gain from this refactoring? Having such corner cases in a test module would also help in making an argument in favor of copying some or even more of this code. A last thing to consider is efficiency. Removing some of the palloc()/pfree() calls may be interesting on this side, actually, numeric can show high in profiles for some workloads. If we can improve some typical scenarios on the way, that makes things easier to sell, and also perhaps easier to shape. I am marking this as RwF. I suspect that there are things that we could do, but I'd suggest to do more than just a copy/paste to satisfy only the case of PL/Haskell you are mentioning. You are obviously doing this move for efficiency reasons in your PL code, I get that, but this needs more thoughts, with perhaps some API redesign to fit not only your case but broader cases that want to play with NumericVar, and consider that with a long-term picture in mind. -- Michael
Attachment
pgsql-hackers by date: