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

[ROQ0fkdz] Reduce the number of transactions we use in triggers #303

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Jan 25, 2023

The ideal solution I suppose is to use @Context public Transaction tx, which calls ProcedureTransactionImpl.

But with both this and @Context publicGraphDatabaseAPI [which calls ProcedureGraphDatabaseAPI] via db.beginTx(),

I receive the following error if I login with an user with role admin
(both with the embedded db and with an enterprise instance):

Creating new node label on database 'system' is not allowed for user '' 
with FULL overridden by READ overridden by READ. See GRANT CREATE NEW NODE LABEL ON DATABASE system

Btw, even trying to explicit GRANT CREATE NEW NODE LABEL ON DATABASE system TO admin gives the same result.

I also tried changing mode = Mode.WRITE to Mode.DBMS but it doesn't change anything.

I think the above one might be a bug, or so, not APOC related.


The only working way is to use @Context public GraphDatabaseAPI, which calls GraphDatabaseFacade.

@vga91 vga91 force-pushed the reduce-triggers-num-transactions branch 2 times, most recently from 6234810 to bd498f4 Compare January 31, 2023 14:32
@vga91 vga91 force-pushed the reduce-triggers-num-transactions branch from bd498f4 to cd22d1d Compare February 13, 2023 09:23
Comment on lines 74 to 75
checkInSystemWriter(tx);
checkTargetDatabase(databaseName, tx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these two you don't need to create a new transaction, you can just recycle the one you are in, so I'd suggest reshaping it like:

        checkInSystemWriter();
        checkTargetDatabase(databaseName);
        Map<String,Object> params = (Map)config.getOrDefault("params", Collections.emptyMap());

        return withTransaction(tx -> {
            TriggerInfo triggerInfo = TriggerHandlerNewProcedures.install(databaseName, name, statement, selector, params, tx);
            return Stream.of(triggerInfo);
        });

so you only open a new system transaction at the moment where you need it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed 👍

@ncordon ncordon force-pushed the reduce-triggers-num-transactions branch from 5993538 to cd22d1d Compare February 23, 2023 16:13
@vga91 vga91 force-pushed the reduce-triggers-num-transactions branch from 9df063c to 05088f7 Compare February 23, 2023 17:05
@vga91 vga91 merged commit f8351cb into dev Feb 23, 2023
@vga91 vga91 deleted the reduce-triggers-num-transactions branch February 23, 2023 17:24
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Mar 7, 2023
…j/apoc#303)

* [ROQ0fkdz] Reduce the number of transactions we use in triggers

* [ROQ0fkdz] Open new transaction only when needed
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Mar 7, 2023
…j/apoc#303)

* [ROQ0fkdz] Reduce the number of transactions we use in triggers

* [ROQ0fkdz] Open new transaction only when needed
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request Mar 7, 2023
…j/apoc#303) (#3482)

* [ROQ0fkdz] Reduce the number of transactions we use in triggers

* [ROQ0fkdz] Open new transaction only when needed
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 this pull request may close these issues.

None yet

2 participants