Skip to content

Conversation

@muhammadshoaib
Copy link
Contributor

@muhammadshoaib muhammadshoaib commented Aug 12, 2022

This is an initial version of AGE for PG12.

Co-authored-by: Alex Kwak emotionbug@apache.org
Co-authored-by: Ibrar Ahmad ibrar.ahmed@percona.com
Co-authored-by: Josh Innis JoshInnis@gmail.com
Co-authored-by: John Gemignani jrgemignani@gmail.com

Copy link

@ibrarahmad ibrarahmad left a comment

Choose a reason for hiding this comment

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

Do we stop providing the upgrade?

&update_indexes = update_result == TM_Ok && !HeapTupleIsHeapOnly(tuple);


// Insert index entries for the tuple

Choose a reason for hiding this comment

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

Don't use "//" comments, it is prohibited in PostgreSQL code. You can always use /* */.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was decided at the beginning of the project to allow // for comments as our product is an extension and not a fork. I don't think this applies to extensions.

Choose a reason for hiding this comment

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

Oh, I don't know, you already decided that. PostgreSQL and all contrib extensions do not use these. Secondly, its for C++ code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, they are not my favorite type of comment. But, it was decided as a group at the beginning of the AGE project, before Apache. Any change would have to be decided as a group. Unless, it were deemed to be necessary to change it.

export PG_CONFIG=$HOME/pg12/bin/pg_config
make -j$(nproc)
make install
make installcheck No newline at end of file

Choose a reason for hiding this comment

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

Every file need to have one blank like at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this?

Choose a reason for hiding this comment

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

Actually, if we follow all PostgreSQL coding rules, it will be easy to run pgident and merge. All of the extensions are doing that. But it's my experience and opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, this means all of PGs coding rules, correct? Including their indentations and use of tabs? We left all that behind at the start of the project.

Choose a reason for hiding this comment

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

Don't worry, we will fix them one by one afterwards, for indentation, I will run pgident that fixes space and indentation issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

What indentation issues? Our coding standard is to use 4 spaces (not a tab) for indentation. Please do not automatically run anything that will reformat files in the AGE repo. Changes like this need to be reviewed by the group before any action is taken.

Choose a reason for hiding this comment

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

I agree, @jrgemignani, that is what we already decided, and I don't want to run any tool without a final agreement. My point is our code is based on PostgreSQL, and for PostgreSQL, even we have copied most of the code from PostgreSQL code. We should make some progress to match PostgreSQL coding guidelines. Not now, but start thinking over that. It will ease our job of the future merging of code, and we can also use PostgreSQL indentation tools in future.

.gitignore Outdated
.idea
.deps
.DS_Store
.DS_Store

Choose a reason for hiding this comment

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

Every file need to have one blank like at the end.

@muhammadshoaib
Copy link
Contributor Author

No, but we don't support the previous releases of AGE for PG12.

2) oid caching code restored

3) whitespace removed
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
@jirutka
Copy link

jirutka commented Aug 22, 2022

updated cypher_kwlist.h

This is not a very useful commit message; there are many (probably unrelated) changes, including formatting changes.

1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
1) regression test restored

2) oid caching code restored

3) whitespace removed

4) copyrights added
@jrgemignani
Copy link
Contributor

Looks good to me. I have no further issues at this time.

Copy link

@ibrarahmad ibrarahmad left a comment

Choose a reason for hiding this comment

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

The commit message is "updated cypher_kwlist.h". and we have changed multiple files in that commit.

* one has this vertex as a start or end vertex.
*/
foreach (lc, labels)
foreach(lc, labels)

Choose a reason for hiding this comment

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

What are these change?

resultRelInfo = create_entity_result_rel_info(
estate, css->set_list->graph_name, label_name);
//relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);

Choose a reason for hiding this comment

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

Don't understand these change?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is probably better to pull the PR down locally and look at it there. There are enough changes made that these diffs are a bit confusing.

@JoshInnis JoshInnis merged commit a20169e into apache:AGE_PG12.1.0_ALPHA Aug 24, 2022
@muhammadshoaib muhammadshoaib deleted the AGE_PG12.1.0_ALPHA branch March 23, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants