Skip to content

chore: implement sql formatter - #306

Open
AndrewJackson2020 wants to merge 5 commits into
masterfrom
sql_formatter
Open

AndrewJackson2020 wants to merge 5 commits into
masterfrom
sql_formatter

Conversation

@AndrewJackson2020

@AndrewJackson2020 AndrewJackson2020 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

This PR implements formatting over the SQL upgrade files. It is implemented via the pgpp binary provided by pglast. I attempted to use 2 other formatters and neither worked.

pgpp is not ideal. It is not all batteries included, had to write a small shell script for the formatting and the checking. Also had to bump nixpkgs to the latest minor release because there was a bug in earlier releases that made this whole thing not work. That said I think that this is our only viable option for now. Also it errors out on sql files that contain only comments but don't if the sql files are totally blank.

I am also totally fine if we don't want to go with what I have done here and would rather wait for pgformatter or postgres-language-server to get fixed.

@AndrewJackson2020
AndrewJackson2020 marked this pull request as ready for review October 2, 2026 20:28
@AndrewJackson2020
AndrewJackson2020 requested a review from a team as a code owner October 2, 2026 20:28

@steve-chavez steve-chavez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TIL about https://github.com/lelit/pglast. Looks good to me 👍

@@ -1 +0,0 @@
-- no SQL changes in 0.11.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I saw this, I think it should be fine to empty the file. I believe I got this pattern from pg_cron.

@AndrewJackson2020 AndrewJackson2020 Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's very annoying that pg_last does not handle this correctly. I think it's debatable whether or not these files should even be included though. pg_duckdb/Jelte's position, for example is that if an upgrade is a change to the C code only then it should not have an upgrade file/control bump. I've never looked at how it's handled in tree. I think because they were included historically, they need to be included for previous releases.

duckdb/pg_duckdb#993

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's nice to be able to include empty files to indicate that there were no sql changes in the new version. Without them the reader wonders if the sql file was forgotten or if it was intentionally left out. I see parallels with the approach where the return value of a function call in Postgres is marked with (void) to indicate that the return value is intentionally ignored (example). But it's not a big deal if not possible with the current tool.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually asked the maintainer and they fixed it. I can redo this to include the comments and the new pglast release.

lelit/pglast#212

@AndrewJackson2020

AndrewJackson2020 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

One thing I notice is that pglast removes comments from the source code. I'm going to try to get it to not do that because that is not ideal. I could see that as being good for bundled code but not development source.

@utkarash2991 @imor Just a heads up this has been approved. Want to give this a few days in case one of y'all have objections. Otherwise will merge this late next week (assuming the comment thing has been fixed).

bash-5.3$ pgpp --help | grep preserve
  -C, --preserve-comments
                        preserve comments in the statement

@imor

imor commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

One thing I notice is that pglast removes comments from the source code. I'm going to try to get it to not do that because that is not ideal. I could see that as being good for bundled code but not development source.

@utkarash2991 @imor Just a heads up this has been approved. Want to give this a few days in case one of y'all have objections. Otherwise will merge this late next week (assuming the comment thing has been fixed).

bash-5.3$ pgpp --help | grep preserve
  -C, --preserve-comments
                        preserve comments in the statement

No objections, go ahead.

This branch has not been deployed

No deployments
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.

4 participants