Bug in create type when missed the comma between element list
Hi team! I found this bug in the create type command. If you miss the comma between the elements, the command doesn't fail; it runs and concatenates the elements. I detected the problem in v14.4, and it is alive in v17. pglatest$ psql -h 127.0.0.1 -p 55532 Password for user daf: Time: 0.481 ms psql (16.4 (Ubuntu 16.4-1.pgdg24.04+2), server 17.0 (Debian 17.0-1.pgdg120+1)) WARNING: psql major version 16, server major version 17. Some psql features might not work. Type "help" for help. u: daf db: daf # CREATE TYPE test_enum AS ENUM( 'one' 'two', 'three', 'four' ); CREATE TYPE Time: 12.242 ms u: daf db: daf # \dt+ Did not find any relations. u: daf db: daf # \dT+ List of data types Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description +---+---+--+--+---+---+- public | test_enum | test_enum | 4 | onetwo +| daf | | | | | | three +| | | | | | | four | | | (1 row) u: daf db: daf # maybe, some of you can help me to report it properly. Thank you, Diego.
Re: Bug in create type when missed the comma between element list
Thank you David! That note: This slightly bizarre behavior is specified by SQL; PostgreSQL is following the standard. On 11/4/24 13:19, David G. Johnston wrote: On Mon, Nov 4, 2024 at 9:17 AM Diego wrote: u: daf db: daf # CREATE TYPE test_enum AS ENUM( 'one' 'two', 'three', 'four' ); maybe, some of you can help me to report it properly. That is working per SQL standard. If you hadn't used newlines between the elements you would have gotten an error; but the newlines between literals is valid string literal syntax. https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS David J.
Re: Conflict Detection and Resolution
Hello hackers, Hey I'm Diego and I do work for Percona and started to work on PostgreSQL and I would like to contribute to the project moving forward. I have been following this thread since the beginning, but due to my limited knowledge of the overall code structure, my first review of the provided patches was more focused on validating the logic and general flow. I have been testing the provided patches and so far the only issue I have is the one reported about DirtySnapshot scans over a B-tree with parallel updates, which may skip/not find some records. That said, I'd like to know if it's worthwhile pulling the proposed fix on [0] and validating/updating the code to fix the issue or if there are other better solutions being discussed? Thanks for your attention, Diego [0]: https://www.postgresql.org/message-id/flat/cantu0oizitbm8+wdtkktmzv0rhgbroygwwqsqw+mzowpmk-...@mail.gmail.com#74f5f05594bb6f10b1d882a1ebce377c On Mon, Oct 21, 2024 at 2:04 AM shveta malik wrote: > On Fri, Oct 18, 2024 at 4:30 PM Zhijie Hou (Fujitsu) > wrote: > > > > On Wednesday, October 9, 2024 2:34 PM shveta malik < > shveta.ma...@gmail.com> wrote: > > > > > > On Wed, Oct 9, 2024 at 8:58 AM shveta malik > > > wrote: > > > > > > > > On Tue, Oct 8, 2024 at 3:12 PM Nisha Moond > > > wrote: > > > > > > > > > > > > > > > Please find few comments on v14-patch004: > > > > > > patch004: > > > 1) > > > GetConflictResolver currently errors out when the resolver is > last_update_wins > > > and track_commit_timestamp is disabled. It means every conflict > resolution > > > with this resolver will keep on erroring out. I am not sure if we > should emit > > > ERROR here. We do emit ERROR when someone tries to configure > > > last_update_wins but track_commit_timestamp is disabled. I think that > should > > > suffice. The one in GetConflictResolver can be converted to WARNING > max. > > > > > > What could be the side-effect if we do not emit error here? In such a > case, the > > > local timestamp will be 0 and remote change will always win. > > > Is that right? If so, then if needed, we can emit a warning saying > something like: > > > 'track_commit_timestamp is disabled and thus remote change is applied > > > always.' > > > > > > Thoughts? > > > > I think simply reporting a warning and applying remote changes without > further > > action could lead to data inconsistencies between nodes. Considering the > > potential challenges and time required to recover from these > inconsistencies, I > > prefer to keep reporting errors, in which case users have an opportunity > to > > resolve the issue by enabling track_commit_timestamp. > > > > Okay, makes sense. We should raise ERROR then. > > thanks > Shveta > > >
Logical replication - proposal for a custom conflict resolution function
Hello hackers, I'd like some feedback on a logical replication feature I would like to write a patch for. The feature would allow users to register a custom conflict handler function to be executed for each conflicting row, after the built-in conflict resolver has run. Running after the built-in conflict resolver allows the function to use the value provided by the resolver, for instance for custom logging. The proposed programming model for the function follows the model for trigger functions - a set of read only variables are populated in the top-level block, and the function returns a record or NULL. The conflict handler function would have more variables. At minimum, three records - the local version of the new value, the remote version of the new value, the value returned by the built-in resolver function - rather than the two for a trigger function, the timestamp information for the local and remote updates, the conflict type detected and the resolver used. The new syntax proposed may look something like this: CREATE SUBSCRIPTION CONNECTION PUBLICATION CONFLICT RESOLVER (conflict_type1 = resolver1 [, conflict_typeN = resolverN,... ]) [conflict_handler=my_custom_function]) // <--- allow users to pass a custom conflict resolution function. Please let me know your thoughts, thanks! Diego
Re: meson vs. llvm bitcode files
Hello, I did a full review on the provided patches plus some tests, I was able to validate that the loading of bitcode modules is working also JIT works for both backend and contrib modules. To test JIT on contrib modules I just lowered the costs for all jit settings and used the intarray extension, using the data/test__int.data: CREATE EXTENSION intarray; CREATE TABLE test__int( a int[] );1 \copy test__int from 'data/test__int.data' For queries any from line 98+ on contrib/intarray/sql/_int.sql will work. Then I added extra debug messages to llvmjit_inline.cpp on add_module_to_inline_search_path() function, also on llvm_build_inline_plan(), I was able to see many functions in this module being successfully inlined. I'm attaching a new patch based on your original work which add further support for generating bitcode from: - Generated backend sources: processed by flex, bison, etc. - Generated contrib module sources, On this patch I just included fmgrtab.c and src/backend/parser for the backend generated code. For contrib generated sources I added contrib/cube as an example. All relevant details about the changes are included in the patch itself. As you may know already I also created a PR focused on llvm bitcode emission on meson, it generates bitcode for all backend and contribution modules, currently under review by some colleagues at Percona: https://github.com/percona/postgres/pull/103 I'm curious if we should get all or some of the generated backend sources compiled to bitcode, similar to contrib modules. Please let me know your thoughts and how we can proceed to get this feature included, thank you. Regards, Diego Fronza Percona On Fri, Mar 7, 2025 at 7:52 AM Nazir Bilal Yavuz wrote: > Hi, > > On Thu, 5 Sept 2024 at 12:24, Nazir Bilal Yavuz > wrote: > > > > I found that Andres shared a patch > > (v17-0021-meson-Add-LLVM-bitcode-emission.patch) a while ago [1]. > > Andres and I continued to work on that. I think the patches are in > sharable state now and I wanted to hear opinions before proceeding > further. After applying the patches, bitcode files should be installed > into $pkglibdir/bitcode/ directory if the llvm is found. > > There are 6 patches attached: > > v1-0001-meson-Add-generated-header-stamps: > > This patch is trivial. Instead of having targets depending directly on > the generated headers, have them depend on a stamp file. The benefit > of using a stamp file is that it makes ninja.build smaller and meson > setup faster. > -- > > v1-0002-meson-Add-postgresql-extension.pc-for-building-extension-libraries: > > This patch is for generating postgresql-extension.pc file which can be > used for building extensions libraries. > > Normally, there is no need to use this .pc file for generating bitcode > files. However, since there is no clear way to get all include paths > for building bitcode files, this .pc file is later used for this > purpose (by running pkg-config --cflags-only-I > postgresql-extension-uninstalled.pc) [1]. > -- > > v1-0003-meson-Test-building-extensions-by-using-postgresql-extension.pc: > [Not needed for generating bitcode files] > > This is a patch for testing if extensions can be built by using > postgresql-extension.pc. I added that commit as an example of using > postgresql-extension.pc to build extensions. > -- > > v1-0004-meson-WIP-Add-docs-for-postgresql-extension.pc: [Not needed > for generating bitcode files] > > I added this patch in case we recommend people to use > postgresql-extension.pc to build extension libraries. I am not sure if > we want to do that because there are still TODOs about > postgresql-extension.pc like running test suites. I just wanted to > show my plan, dividing 'Extension Building Infrastructure' into two, > 'PGXS' and 'postgresql-extension.pc'. > -- > > v1-0005-meson-Add-LLVM-bitcode-emission: > > This patch adds required infrastructure to generate bitcode files and > uses postgresql-extension-uninstalled.pc to get include paths for > generating bitcode files [1]. > -- > > v1-0006-meson-Generate-bitcode-files-of-contrib-extension.patch: > > This patch adds manually selected contrib libraries to generate their > bitcode files. These libraries are selected manually, depending on > - If they have SQL callable functions > - If the library functions are short enough (the performance gain from > bitcode files is too minimal compared to the function's run time, so > this type of libraries are omitted). > > Any kind of feedback would be appreciated. > > -- > Regards, > Nazir Bilal Yavuz > Microsoft > From be09b1f38d107789fc9ffe1cd2b2470552689a20 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Mon
Re: meson vs. llvm bitcode files
Hi,
The v7 patch looks good to me, handling the bitcode modules in a uniform
way and also avoiding the hacky code and warnings, much better now.
A small note about the bitcode emission for generated sources in contrib,
using cube as example, currently it creates two dict entries in a list:
bc_seg_gen_sources = [{'srcfiles': [seg_scan]}]
bc_seg_gen_sources += {'srcfiles': [seg_parse[0]]}
Then pass it to the bitcode_modules:
bitcode_modules += {
...
'gen_srcfiles': bc_seg_gen_sources,
}
It could be passed as a list with a single dict, since both generated
sources share the same compilation flags:
bitcode_modules += {
...
'gen_srcfiles': [
{ 'srcfiles': [cube_scan, cube_parse[0]] }.
]
}
Both approaches work, the first one has the advantage of being able to pass
separate additional_flags per generated source.
Thanks for your reply Nazir, also waiting for more opinions on this.
Regards,
Diego
On Wed, Mar 12, 2025 at 7:27 AM Nazir Bilal Yavuz
wrote:
> Hi,
>
> On Tue, 11 Mar 2025 at 01:04, Diego Fronza
> wrote:
> > I did a full review on the provided patches plus some tests, I was able
> to validate that the loading of bitcode modules is working also JIT works
> for both backend and contrib modules.
>
> Thank you!
>
> > To test JIT on contrib modules I just lowered the costs for all jit
> settings and used the intarray extension, using the data/test__int.data:
> > CREATE EXTENSION intarray;
> > CREATE TABLE test__int( a int[] );1
> > \copy test__int from 'data/test__int.data'
> >
> > For queries any from line 98+ on contrib/intarray/sql/_int.sql will work.
> >
> > Then I added extra debug messages to llvmjit_inline.cpp on
> add_module_to_inline_search_path() function, also on
> llvm_build_inline_plan(), I was able to see many functions in this module
> being successfully inlined.
> >
> > I'm attaching a new patch based on your original work which add further
> support for generating bitcode from:
>
> Thanks for doing that!
>
> > - Generated backend sources: processed by flex, bison, etc.
> > - Generated contrib module sources,
>
> I think we do not need to separate these two.
>
>foreach srcfile : bitcode_module['srcfiles']
> -if meson.version().version_compare('>=0.59')
> +srcfilename = '@0@'.format(srcfile)
> +if srcfilename.startswith(' + srcfilename = srcfile.full_path().split(meson.build_root() + '/')[1]
> +elif meson.version().version_compare('>=0.59')
>
> Also, checking if the string starts with ' hacky, and 'srcfilename = '@0@'.format(srcfile)' causes a deprecation
> warning. So, instead of this we can process all generated sources like
> how generated backend sources are processed. I updated the patch with
> that.
>
> > On this patch I just included fmgrtab.c and src/backend/parser for the
> backend generated code.
> > For contrib generated sources I added contrib/cube as an example.
>
> I applied your contrib/cube example and did the same thing for the
> contrib/seg.
>
> > All relevant details about the changes are included in the patch itself.
> >
> > As you may know already I also created a PR focused on llvm bitcode
> emission on meson, it generates bitcode for all backend and contribution
> modules, currently under review by some colleagues at Percona:
> https://github.com/percona/postgres/pull/103
> > I'm curious if we should get all or some of the generated backend
> sources compiled to bitcode, similar to contrib modules.
>
> I think we can do this. I added other backend sources like you did in
> the PR but attached it as another patch (0007) because I wanted to
> hear other people's opinions on that first.
>
> v3 is attached.
>
> --
> Regards,
> Nazir Bilal Yavuz
> Microsoft
>
The first function call
Hello, The first function call is 10 times slower than the other calls in the same session. Is it possible to shorten this long time on the first call? For example. Call my function for once, this call returns at 70ms on the next call, the return is at 7ms. thanks.
