Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing parameters in the arg_walker callback of raw_expression_tree_walker ? #1583

Closed
daamien opened this issue Feb 22, 2024 · 5 comments · Fixed by #1596
Closed

Missing parameters in the arg_walker callback of raw_expression_tree_walker ? #1583

daamien opened this issue Feb 22, 2024 · 5 comments · Fixed by #1596

Comments

@daamien
Copy link
Contributor

daamien commented Feb 22, 2024

Hi !

This may not be a bug but it is at least very weird :)

I'm trying to use the raw_expression_tree_walker function, but I don't understant why the arg_walker callback function as no parameters... It's signature is

unsafe extern C fn() -> bool

https://docs.rs/pgrx/latest/pgrx/pg_sys/fn.raw_expression_tree_walker.html

This callback seems useless if I can't inspect the node itself...

I would expect the walker callback signature to be

unsafe extern C fn(* mut Node, * c_void) -> bool

Looking at the example below in the PostgreSQL code, I see that the tree_walker_callback used in raw_expression_tree_walker does have 2 parameters....

https://github.com/postgres/postgres/blob/master/src/backend/parser/analyze.c#L3576

Am I missing something obvious ?

@workingjubilee
Copy link
Member

@daamien Mostly that Postgres 16 diffs this: postgres/postgres@1c27d16

@workingjubilee
Copy link
Member

@daamien I am happy to accept a PR that does the relevant mischief to make all versions conform to the interface that Postgres 16 describes via the appropriate function-pointer casting/transmutes.

@daamien
Copy link
Contributor Author

daamien commented Feb 28, 2024

I'm afraid this is way beyond my capabilities :/

workingjubilee added a commit that referenced this issue Mar 1, 2024
These functions had been renamed in Postgres 16, and they are now fn
macros. That means this `extern "C"` declaration is simply incorrect in
Postgres 16. Implement a relatively logical version-by-version handling.

Fixes #1583.
@daamien
Copy link
Contributor Author

daamien commented Apr 1, 2024

Hi @workingjubilee

As I'm moving forward with this, I realize that PR #1596 does cover query_tree_walker, expression_tree_walker and raw_expression_tree_walker but there's a few more functions that are also concerned by PG16's signature change

  • query_or_expression_tree_walker
  • range_table_entry_walker
  • planstate_tree_walker
  • range_table_walker

As far as I can tell, with their current signature those 4 functions a pretty much useless at the moment.

Now I guess I should be able to copycat what you did on #1596 and submit a patch to fix those 4 functions.... Should I make a single PR for them all or a distinct PR for each of them ?

@workingjubilee
Copy link
Member

@daamien Feel free to take all of them in one.

workingjubilee added a commit that referenced this issue Apr 5, 2024
As a follow up to the discussion here 

#1583 (comment)

Here's a patch to declare correctly the following functions:
* planstate_tree_walker
* query_or_expression_tree_walker
* range_table_entry_walker
* range_table_walker

This is a shameless copy of PR #1596 except for making sure
the argument names match the C function argument names.

Co-authored-by: Jubilee Young <workingjubilee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants