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

Please work around limitation of 100 or 50 fields for the function parameters such as agtype_build_map and internally invoked CREATE #1840

Open
MironAtHome opened this issue May 8, 2024 · 13 comments
Labels
enhancement New request Stale Stale issues/PRs

Comments

@MironAtHome
Copy link

MironAtHome commented May 8, 2024

Is your feature request related to a problem? Please describe.
When working with data having vertice property count in excess of 100 Postgres functions exhibit build - in limit of 100 arguments.
Examples:
function agtype_build_map treats each parameter as separate, map field name and map field value, hence, it limits input to 50 fields / call. To scale number of fields it can handle one has to continuously invoke constructs such as
agtype_add(
CypherInputParam_Var
, agtype_build_map(
/* 54 / field_name_54, field_value_54
/
55 */ field_name_55, field_value_55
....
similarly when CREATE invoked subsequently in the context of call to cypher function and CypherInputParam_Var as an input parameter to overcome per call limit of parameters/properties requires to initially create subset of 50 fields and subsequently call constructs such as
CREATE (v:<vertice_name>
{... properties 1,2,3 ...
})
SET v += { properties 51... 100}
SET v += { properties 101... 150}
....
This limitation is grandfathered from functions servicing postgresql function check when using default build, as per Appendix K. PostgreSQL Limits document:

function arguments | 100 | can be increased by recompiling PostgreSQL

However, I am not 100% convinced that custom compilation of Postgres is the right approach in this case.
In an ideal world it would be nice if Age used under the hood an limit compatible with limit of Age, which I believe is 65,535 properties per vertice ( please correct me if I am wrong ).

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@MironAtHome MironAtHome added the enhancement New request label May 8, 2024
@jrgemignani
Copy link
Contributor

jrgemignani commented May 8, 2024

@MironAtHome I can try to look into this tomorrow, time permitting. I recall something, somewhere, about this limit and creating a workaround. But, it's been a while.

Edit: Related to PR #1001

@jrgemignani
Copy link
Contributor

@MironAtHome It looks like transform_cypher_map function could use an update.

@jrgemignani
Copy link
Contributor

jrgemignani commented May 30, 2024

@MironAtHome PR #1901 will fix this for objects, but not for SET. It is currently in review.

@MironAtHome
Copy link
Author

MironAtHome commented Jun 25, 2024

Just tried the fix.
Function agtype_build_map isn't fixed.
Getting message:
ERROR: cannot pass more than 100 arguments to a function
LINE 1: CypherInputParam_Var := agtype_build_map(
^
So, I am assuming it's this function that is failing 100 argument check, and, if my understanding is correct, it is checked on entry, before function can start processing.
I am putting together an jsonb array handler.
Ideally we should also have something, where we put a record in, in the style of to_jsonb, to_agtype and get back map, with proper types. Not sure column naming rules fit property name of agtype, again, if you remember, recent ask regarding validation function, would come handy, ideally, we should extend something like a "default name normalizing" utility, with an option for the user to supply custom function, if they need some names to have special translation rules.
Array is simpler, I just fetch in name and value.
Could it be these functions already present and I missed?
There is always a chance. In this case would appreciate an advice.

@jrgemignani
Copy link
Contributor

@MironAtHome This is only fixed for agtype_build_map in the branches and not for SET. The releases don't have the fix. You would need to build from one of the branches.

@MironAtHome
Copy link
Author

Took changes from branch PG16.
Will try to figure out what's wrong and get back.

@MironAtHome
Copy link
Author

Checking regression test I am seeing the following. The call to function agtype_build_map seems to be missing:
image

@MironAtHome
Copy link
Author

MironAtHome commented Jun 25, 2024

Have ran above scripts on the server with the build, which included fixes from branch PG16 and all completed successfully.
So, it does look like cypher parser is accepting 100+ parameters for various commands, however, the call to function agtype_biuld_map fails.
Looks like my original intent to add function call with jsonb block as input will be of use.
I may still need to check, if it is going to be array or set of json properties. The goal is to have something like

for m
in select jsonb_object_agg(*) from t
loop
    agtype_map_input_parameter := jsonb_to_agtype_map(m);
    select into output_var
              t.a
   from cypher ('my_graph', $$
      CREATE (v:MyVertice { p1:$p1, .... })
   $$, agtype_map_input_parameter) AS t(a agtype);
end loop

without having to split map building call into many - many "add_agtype(m, new_set)" calls.
The fix for CREATE call certainly helps greatly! Thank you.
To go further, having red some stack overflow entries on the merit, frequent comment from accomplished professionals "if you need function with more than 100 parameters, something is wrong with your design".
Well, it might work in academic work of student, but something like an reporting application may need to handle 15,000 columns in a single row, easily.
So, it's time to mature infrastructure level. And since Postgres seems to be hard set on keeping limit of parameters, array / set of properties seems like a simple workaround, and a bit further down the road, row wise conversion to graph vertice or set of entries in the vertice, with row and / or table as input parameter seems like an appropriate architectural feature addition.

@MironAtHome
Copy link
Author

I have put together function
jsonb_to_agtype_map
that works for me. Will share it via same branch for PG14 and PG16 ports as before.
Please see if those have code that can be included or as an idea to improve upon.
Yeah, ideally, it would be nice to have one more function converting row to map, without having to use "to_jsonb" in between.

@jrgemignani
Copy link
Contributor

So, it does look like cypher parser is accepting 100+ parameters for various commands, however, the call to function agtype_biuld_map fails.

@MironAtHome Unfortunately, there is nothing that I can do to change PG's limit on function parameters. The only reason that I was able to "fix" it for some Cypher commands is because the transform logic allows us to code around it, because we can control how to build the function parameter list. But, we still have to look at each case in order to do that. Using a function directly will always run up against that limit.

@MironAtHome
Copy link
Author

MironAtHome commented Jun 27, 2024

I hope this function jsonb_to_agtype_map_worker for branch PG14 here is a good starting point to start adding functions that are able to add functionality, without running into function limitation
Branch PG16 has identical function code here
With very limited test and to the extent what needed right here and right now this code provides. However, I can think of, potentially, a number of, ways to examine this function with an eye out to better type handling and, possibly, more optimal flow of conversion of data from jsonb to agtype.
Once again, extending similar functionality to cover row as input, styled after postgres native "to_jsonb" and, possibly, handler for jsonb array, looks to me like a nice follow up TODO list.
Here is basic test:

`create extension if not exists age;
LOAD 'age';
SET search_path TO ag_catalog;

SELECT jsonb_to_agtype_map(to_jsonb(t)) from (
select 1 AS Col1
, 2 AS Col2
, 3 AS Col3
, 4 AS Col4
, 5 AS Col5
, 6 AS Col6
, 7 AS Col7
, 8 AS Col8
, 9 AS Col9
, 10 AS Col10
, 11 AS Col11
, 12 AS Col12
, 13 AS Col13
, 14 AS Col14
, 15 AS Col15
, 16 AS Col16
, 17 AS Col17
, 18 AS Col18
, 19 AS Col19
, 20 AS Col20
, 21 AS Col21
, 22 AS Col22
, 23 AS Col23
, 24 AS Col24
, 25 AS Col25
, 26 AS Col26
, 27 AS Col27
, 28 AS Col28
, 29 AS Col29
, 30 AS Col30
, 31 AS Col31
, 32 AS Col32
, 33 AS Col33
, 34 AS Col34
, 35 AS Col35
, 36 AS Col36
, 37 AS Col37
, 38 AS Col38
, 39 AS Col39
, 40 AS Col40
, 41 AS Col41
, 42 AS Col42
, 43 AS Col43
, 44 AS Col44
, 45 AS Col45
, 46 AS Col46
, 47 AS Col47
, 48 AS Col48
, 49 AS Col49
, 50 AS Col50
, 51 AS Col51
, 52 AS Col52
, 53 AS Col53
, 54 AS Col54
, 55 AS Col55
, 56 AS Col56
, 57 AS Col57
, 58 AS Col58
, 59 AS Col59
, 60 AS Col60
, 61 AS Col61
, 62 AS Col62
, 63 AS Col63
, 64 AS Col64
, 65 AS Col65
, 66 AS Col66
, 67 AS Col67
, 68 AS Col68
, 69 AS Col69
, 70 AS Col70
, 71 AS Col71
, 72 AS Col72
, 73 AS Col73
, 74 AS Col74
, 75 AS Col75
, 76 AS Col76
, 77 AS Col77
, 78 AS Col78
, 79 AS Col79
, 80 AS Col80
, 81 AS Col81
, 82 AS Col82
, 83 AS Col83
, 84 AS Col84
, 85 AS Col85
, 86 AS Col86
, 87 AS Col87
, 88 AS Col88
, 89 AS Col89
, 90 AS Col90
, 91 AS Col91
, 92 AS Col92
, 93 AS Col93
, 94 AS Col94
, 95 AS Col95
, 96 AS Col96
, 97 AS Col97
, 98 AS Col98
, 99 AS Col99
, 100 AS Col100
, 101 AS Col101
) as t;`

@MironAtHome
Copy link
Author

MironAtHome commented Jul 17, 2024

Branch was updated with one more function
datum_to_agtype_map
to perform similar work, except it now does not require intermediate step of converting query result into jsonb type variable and, on my mind, should be better with regards to type safety, since handling of variable type is now direct
column -> agtype map, with field name assumed is the key / value is the value.
as opposed to field -> json key/value -> agtype key / value with conversion requirements of json - to -> agtype being somewhat unclear.
I didn't have time beyond minimal test, but the results were encouraging. I will see if 1600 field limit applies to this function, as it does to postgres table, and if it is, it's a lot more than 50 key / value pairs, support by agtype_build_map right now, and if it turns out friendly to higher number of fields, it would mean that with this function Age can support analytical and reporting
very-very well, somewhat upping Postgres into this exciting area. With 65535 overall field limit for vertice property count on one hand, and 1MB limit to what can handle jsonb variable, it would be worth a quick check.

LOAD 'age';
SET search_path = ag_catalog, "$user", public;
DO
$RUN$
DECLARE
    AGMap_Var agtype;
BEGIN
	LOAD 'age';
    SET SEARCH_PATH TO ag_catalog;
    WITH t AS (
		SELECT 1 AS Col1
		    , 'a' AS Col2
            , '\xffef'::bytea AS Col3
            , (current_timestamp)::timestamp AS Col4
            , '2024-01-01'::date AS Col5
            , '2024-01-01 10:00:00'::time AS Col6
            , 'abc'::text AS Col7
            , 10.1::float AS Col8
            , 10.1::integer AS Col9
            , 10.1::bigint AS Col10
            , 11 AS Col11
            , 12 AS Col12
            , 13 AS Col13
            , 14 AS Col14
            , 15 AS Col15
            , 16 AS Col16
            , 17 AS Col17
            , 18 AS Col18
            , 19 AS Col19
            , 20 AS Col20
            , 21 AS Col21
            , 22 AS Col22
            , 23 AS Col23
            , 24 AS Col24
            , 25 AS Col25
            , 26 AS Col26
            , 27 AS Col27
            , 28 AS Col28
            , 29 AS Col29
            , 30 AS Col30
            , 31 AS Col31
            , 32 AS Col32
            , 33 AS Col33
            , 34 AS Col34
            , 35 AS Col35
            , 36 AS Col36
            , 37 AS Col37
            , 38 AS Col38
            , 39 AS Col39
            , 40 AS Col40
            , 41 AS Col41
            , 42 AS Col42
            , 43 AS Col43
            , 44 AS Col44
            , 45 AS Col45
            , 46 AS Col46
            , 47 AS Col47
            , 48 AS Col48
            , 49 AS Col49
            , 50 AS Col50
            , 51 AS Col51
            , 52 AS Col52
            , 53 AS Col53
            , 54 AS Col54
            , 55 AS Col55
            , 56 AS Col56
            , 57 AS Col57
            , 58 AS Col58
            , 59 AS Col59
            , 60 AS Col60
            , 61 AS Col61
            , 62 AS Col62
            , 63 AS Col63
            , 64 AS Col64
            , 65 AS Col65
            , 66 AS Col66
            , 67 AS Col67
            , 68 AS Col68
            , 69 AS Col69
            , 70 AS Col70
            , 71 AS Col71
            , 72 AS Col72
            , 73 AS Col73
            , 74 AS Col74
            , 75 AS Col75
            , 76 AS Col76
            , 77 AS Col77
            , 78 AS Col78
            , 79 AS Col79
            , 80 AS Col80
            , 81 AS Col81
            , 82 AS Col82
            , 83 AS Col83
            , 84 AS Col84
            , 85 AS Col85
            , 86 AS Col86
            , 87 AS Col87
            , 88 AS Col88
            , 89 AS Col89
            , 90 AS Col90
            , 91 AS Col91
            , 92 AS Col92
            , 93 AS Col93
            , 94 AS Col94
            , 95 AS Col95
            , 96 AS Col96
            , 97 AS Col97
            , 98 AS Col98
            , 99 AS Col99
            , 100 AS Col100
            , 101 AS Col101
	)
    SELECT INTO AGMap_Var 
	       ag_catalog.datum_to_agtype_map(t)
	FROM t AS t;
	RAISE NOTICE 'AGMap_Var : %', AGMap_Var;
END;
$RUN$;

Please see if the two functions, datum_to_agtype_map and jsonb_to_agtype_map can be promoted to age master.

Copy link

This issue is stale because it has been open 60 days with no activity. Remove "Abondoned" label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale Stale issues/PRs label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New request Stale Stale issues/PRs
Projects
None yet
Development

No branches or pull requests

2 participants