-
Notifications
You must be signed in to change notification settings - Fork 110
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
Overhaul the documentation #125
Conversation
This wasn't really maintained. A following commit will instead setup a markdown documentation, which can be used to generate proper HTML documentation if needed, and host it on some third-party service if needed.
`pg_hint_plan` stops parsing on any error and uses hints already parsed on the | ||
most cases. Following are the typical errors. | ||
|
||
#### Syntax errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use three '#' here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that it's a subclass or the general "Errors" before. If we change it to be 1 level higher we should move it after "Nested Comments", or before "Errors".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just looked at a good half of the docs and noticed a lot of typos, so the docs really need more work. I think that this could just be done as a follow-up change, once the split and indentation of the docs make it much easier to follow grammar corrections.
`pgbench_accounts` by sequential scan method. | ||
|
||
```sql | ||
postgres=# /*+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something a bit annoying with this style is that queries cannot be easily query-pasted. Could it be possible to minimize the number of "postgres#" here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's a bit annoying, both for readability and copy/pastability (not all editors/browser support block-mode copying), so +1 to switch to an empty PROMPT2 style.
|
||
`pg_hint_plan` reads hinting phrases in a comment of special form given with | ||
the target SQL statement. The special form is beginning by the character | ||
sequence `"/\*+"` and ends with `"\*/"`. Hint phrases are consists of hint name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Hint phrases consist of hint names, parameters enclosed by parenthesis and delimited by whitespaces"?
## Errors | ||
|
||
`pg_hint_plan` stops parsing on any error and uses hints already parsed on the | ||
most cases. Following are the typical errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Some of the typical errors"?
|
||
#### Object misspecifications | ||
|
||
Object misspecifications result in silent ignorance of the hints. This kind of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Object misspells result in silently ignoring the hints."
|
||
#### Redundant or conflicting hints | ||
|
||
The last hint will be active when redundant hints or hints conflicting with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the event of a redundant or conflicting hint, the last hint will be the one considered.
|
||
#### Nested comments | ||
|
||
Hint comment cannot include another block comment within. If `pg_hint_plan` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hint blocks, written as comments, cannot include an extra comment block within themselves.
#### Nested comments | ||
|
||
Hint comment cannot include another block comment within. If `pg_hint_plan` | ||
finds it, differently from other erros, it stops parsing and abandans all hints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence should be reworked.
#### Queries in ECPG | ||
|
||
ECPG removes comments in queries written as embedded SQLs so hints cannot be | ||
passed form those queries. The only exception is that `EXECUTE` command passes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/form/from/
|
||
ECPG removes comments in queries written as embedded SQLs so hints cannot be | ||
passed form those queries. The only exception is that `EXECUTE` command passes | ||
given string unmodifed. Please consider using the hint table in the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/unmodifed/unmodified/.
Patch 1 is a straight-forward removal. Nothing to comment on that. In short, +1 ;) It really looks like the docs in English should be reworked and trimmed from all the incorrectness I found while reading them. Fine by me to do it once the split is done, and the reindentation makes that so muuuuch easier. |
I also found some typos when reformatting the docs (I pushed a few commits for that), and there are indeed a lot more. It will be much easier to work on that after merging this PR as it seems like a big project on its own, and as I also think that a 80-col width makes things easier :) |
About back-patching, is pg14 a decent enough target? Older versions don't have a README, and going from html to md isn't really motivating me. |
My motivation would not go beyond the stable branch where I begin to see conflicts, so agreed that this should be OK. Things have not changed much in the last few years, still there may be a delta. |
I was thinking of doing the same steps manually on each branches, as they all conflict and resolving them seems more dangerous than copy/pasting stuff again. |
I've been discussing with @yamatattsu and keeping a multi-language documentation seems to be something quite likely. I've been doing some more research and mkdocs isn't really as mature as sphinx when it comes to such feature. I will work on a new branch trying to build the same thing using sphinx and the myst-parser to keep most of the documentation in markdown. Note that while markdown is way more readable, it's also way less powerful than restructured text. Going with sphinx might be a better option in the long run as we would be able to switch to restructured text more easily if we ever need to, even on a per-file basis. |
Markdown format is kept as a main format, relying on python sphinx and myst_parser to render an HTML documentation if needed. Note that while markdown is more readable as raw text, it's a way simpler syntax that lacks a lot of features that reStructuredText offers. Relying on sphinx gives us an opportunity to later write parts of the documentatin in reStructuredText if needed, but also offers other appealing features like multilingual documentation that we may reintroduce in a near future. Readthedocs is the expected target, so use its theme and follow its recommendation about pinning various requirement versions. The documentation can be built locally easily using make -C docs/ html The rendered documentation will be generated in docs/html/_build/html Note that you need to have all python prerequirements installed, which can be done using: pip install -r docs/requirements.txt If you need to update the requirements (which shouldn't be needed frequently), update the docs/requirements.in and generate the target docs/requirements.txt using pip-compile. See the link about this tool below for more details on how to use it. As a first step, simply duplicate the README.md for the documentation. References if you're interested in the various design choices: - quickstart for RTD with sphinx: https://docs.readthedocs.io/en/stable/intro/getting-started-with-sphinx.html - reproducible builds: https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html - myst parser: https://myst-parser.readthedocs.io - pip-tools / pip-compile: https://pip-tools.readthedocs.io - RTD sphinx theme: https://sphinx-rtd-theme.readthedocs.io
Use a 80 characters line limit, and use sql block code to get slightly better syntax highlighting.
We rely on python sphinx-intl to handle the translation, using standard .pot / .po translation files. The following steps are required each time the translation needs to be updated: make gettext sphinx-intl update -p _build/gettext -l ja Translators can then work on the various .po files generated (or updated) in docs/locale/ja/LC_MESSAGES/, and the translated documentation can be built using: make -e SPHINXOPTS="-D language='ja'" html Once everything is ok, simply commit the modified files in docs/locale. Note that we don't track the .pot files, generated in docs/_build/gettext/.
Sorry for the delay, I didn't have time to work on that before today. So I now have another patchset that relies on sphinx but keeps markdown compatibility thanks to the myst-parser plugin. I have to say that I've been pleasantly surprised by this combination. I wanted to make sure that every features we're interested in would work so I also added the basis for Japanese translation, and imported a few bits of the previous translation, mostly where the English version didn't change at all or the chunks I could understand. The updated result can be seen at https://pg-hint-plan.readthedocs.io/en/latest/ The result is much better as we can now rely on the TOC being automatically generated. It's however a bit inconsistent as the included page should start as a nested level of 1 rather than 2, but before spending time cleaning that up I want to make sure that everyone is onboard with this new version. Note also that the version selector (bottom left) now shows a language select, with en and ja available, the Japanese version available at https://pg-hint-plan.readthedocs.io/ja/latest/. I used a new branch for this work, available at https://github.com/rjuju/pg_hint_plan/commits/doc_v2, so you can easily compare the differences in the structure. While at it I added a lot of internal documentation on how to work with the documentation, so hopefully it should be easy to get started with either building the doc locally or eventually finishing the translation. Let me know what you think, and if I should forcepush on this branch to update the PR or just create a new one with the sphinx approach. |
Force pushing on this PR is fine by me. |
Ok, done! |
FTR I pushed a naive "doc_PG15" version for en and ja, which just changed the version info in the main page to also test the multi version, and it works as expected. The version select now shows either "latest" and "doc_PG15" (which is the branch name, once merged it will just be PG15), for both en and ja versions. As far as I'm concerned I'm happy with this tooling and I think it's the best compromise we can aim for, as we will be ready for any other fancy feature integration in the future if needed. That being said, I've worked on the pg translation for years so I'm already quite used to working with po files, which may not be the case for people interested in translating this documentation. |
|
||
Note that while markdown is more readable as raw text, it's a way simpler | ||
syntax that lacks a lot of features that reStructuredText offers. Relying on | ||
sphinx gives us an opportunity to later write parts of the documentatin in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/documentatin/documentation/
@rjuju I have been reading through your set of changes, and a good portion of them are unchanged compared to the last version, at the exception of the documentation about how to use sphinx. +1 to apply what you have here, and work towards improving things in this direction. |
Markdown format is kept as a main format, relying on python sphinx and myst_parser to render an HTML documentation if needed. Note that while markdown is more readable as raw text, it's a way simpler syntax that lacks a lot of features that reStructuredText offers. Relying on sphinx gives us an opportunity to later write parts of the documentation in reStructuredText if needed, but also offers other appealing features like multilingual documentation that we may reintroduce in a near future. Readthedocs is the expected target, so use its theme and follow its recommendation about pinning various requirement versions. The documentation can be built locally easily using make -C docs/ html The rendered documentation will be generated in docs/html/_build/html Note that you need to have all python prerequirements installed, which can be done using: pip install -r docs/requirements.txt If you need to update the requirements (which shouldn't be needed frequently), update the docs/requirements.in and generate the target docs/requirements.txt using pip-compile. See the link about this tool below for more details on how to use it. As a first step, simply duplicate the README.md for the documentation. References if you're interested in the various design choices: - quickstart for RTD with sphinx: https://docs.readthedocs.io/en/stable/intro/getting-started-with-sphinx.html - reproducible builds: https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html - myst parser: https://myst-parser.readthedocs.io - pip-tools / pip-compile: https://pip-tools.readthedocs.io - RTD sphinx theme: https://sphinx-rtd-theme.readthedocs.io Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
We rely on python sphinx-intl to handle the translation, using standard .pot / .po translation files. The following steps are required each time the translation needs to be updated: make gettext sphinx-intl update -p _build/gettext -l ja Translators can then work on the various .po files generated (or updated) in docs/locale/ja/LC_MESSAGES/, and the translated documentation can be built using: make -e SPHINXOPTS="-D language='ja'" html Once everything is ok, simply commit the modified files in docs/locale. Note that we don't track the .pot files, generated in docs/_build/gettext/. Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
Now that the docs are split in different files, we need to add myst-parser specific markups for cross-reference links. After this patch, there are still 2 problems reported when building the docs: - the snippet in docs/hint_details.md creating hints_func errors out as the "$" character is incompatible with SQL formatting. Leave it as-is, as the result is that sql highlighting is only skipped for this snippet, which would be the same if we explicitly removed the sql tag, but we still get a warning to remind us about the problem if we want to eventually find a solution. - there's a link to some "#restrictions" paragraph in docs/hint_table.md, but as far as I can see this link has always been dead so I'm not sure what to do of it. Pull request #125, fixes #123 Backpatch-through: 14
Markdown format is kept as a main format, relying on python sphinx and myst_parser to render an HTML documentation if needed. Note that while markdown is more readable as raw text, it's a way simpler syntax that lacks a lot of features that reStructuredText offers. Relying on sphinx gives us an opportunity to later write parts of the documentation in reStructuredText if needed, but also offers other appealing features like multilingual documentation that we may reintroduce in a near future. Readthedocs is the expected target, so use its theme and follow its recommendation about pinning various requirement versions. The documentation can be built locally easily using make -C docs/ html The rendered documentation will be generated in docs/html/_build/html Note that you need to have all python prerequirements installed, which can be done using: pip install -r docs/requirements.txt If you need to update the requirements (which shouldn't be needed frequently), update the docs/requirements.in and generate the target docs/requirements.txt using pip-compile. See the link about this tool below for more details on how to use it. As a first step, simply duplicate the README.md for the documentation. References if you're interested in the various design choices: - quickstart for RTD with sphinx: https://docs.readthedocs.io/en/stable/intro/getting-started-with-sphinx.html - reproducible builds: https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html - myst parser: https://myst-parser.readthedocs.io - pip-tools / pip-compile: https://pip-tools.readthedocs.io - RTD sphinx theme: https://sphinx-rtd-theme.readthedocs.io Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
We rely on python sphinx-intl to handle the translation, using standard .pot / .po translation files. The following steps are required each time the translation needs to be updated: make gettext sphinx-intl update -p _build/gettext -l ja Translators can then work on the various .po files generated (or updated) in docs/locale/ja/LC_MESSAGES/, and the translated documentation can be built using: make -e SPHINXOPTS="-D language='ja'" html Once everything is ok, simply commit the modified files in docs/locale. Note that we don't track the .pot files, generated in docs/_build/gettext/. Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
Now that the docs are split in different files, we need to add myst-parser specific markups for cross-reference links. After this patch, there are still 2 problems reported when building the docs: - the snippet in docs/hint_details.md creating hints_func errors out as the "$" character is incompatible with SQL formatting. Leave it as-is, as the result is that sql highlighting is only skipped for this snippet, which would be the same if we explicitly removed the sql tag, but we still get a warning to remind us about the problem if we want to eventually find a solution. - there's a link to some "#restrictions" paragraph in docs/hint_table.md, but as far as I can see this link has always been dead so I'm not sure what to do of it. Pull request #125, fixes #123 Backpatch-through: 14
Markdown format is kept as a main format, relying on python sphinx and myst_parser to render an HTML documentation if needed. Note that while markdown is more readable as raw text, it's a way simpler syntax that lacks a lot of features that reStructuredText offers. Relying on sphinx gives us an opportunity to later write parts of the documentation in reStructuredText if needed, but also offers other appealing features like multilingual documentation that we may reintroduce in a near future. Readthedocs is the expected target, so use its theme and follow its recommendation about pinning various requirement versions. The documentation can be built locally easily using make -C docs/ html The rendered documentation will be generated in docs/html/_build/html Note that you need to have all python prerequirements installed, which can be done using: pip install -r docs/requirements.txt If you need to update the requirements (which shouldn't be needed frequently), update the docs/requirements.in and generate the target docs/requirements.txt using pip-compile. See the link about this tool below for more details on how to use it. As a first step, simply duplicate the README.md for the documentation. References if you're interested in the various design choices: - quickstart for RTD with sphinx: https://docs.readthedocs.io/en/stable/intro/getting-started-with-sphinx.html - reproducible builds: https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html - myst parser: https://myst-parser.readthedocs.io - pip-tools / pip-compile: https://pip-tools.readthedocs.io - RTD sphinx theme: https://sphinx-rtd-theme.readthedocs.io Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
We rely on python sphinx-intl to handle the translation, using standard .pot / .po translation files. The following steps are required each time the translation needs to be updated (with docs/ as current working directory): make gettext sphinx-intl update -p _build/gettext -l ja Translators can then work on the various .po files generated (or updated) in docs/locale/ja/LC_MESSAGES/, and the translated documentation can be built using: make -e SPHINXOPTS="-D language='ja'" html Once everything is ok, simply commit the modified files in docs/locale. Note that we don't track the .pot files, generated in docs/_build/gettext/. Pull request #125, fixes #123 Backpatch-through: 14 Reviewed-by: Michael Paquier
Now that the docs are split in different files, we need to add myst-parser specific markups for cross-reference links. After this patch, there are still 2 problems reported when building the docs: - the snippet in docs/hint_details.md creating hints_func errors out as the "$" character is incompatible with SQL formatting. Leave it as-is, as the result is that sql highlighting is only skipped for this snippet, which would be the same if we explicitly removed the sql tag, but we still get a warning to remind us about the problem if we want to eventually find a solution. - there's a link to some "#restrictions" paragraph in docs/hint_table.md, but as far as I can see this link has always been dead so I'm not sure what to do of it. Pull request #125, fixes #123 Backpatch-through: 14
Ok! So I took care of doing some additional fixups to remove doc build warnings, and manually did the same work for PG14 and PG15 and pushed the result on all branches! The next thing is to know whether we want to host compiled documentation on RTD or not. If yes, I need the list of person and their RTD accounts to give permission (and possibly remove my own access if needed), and also someone then needs to add the proper webhooks on this repository. It requires more permissions than I have, and can be done easily using RTD UI. |
Should this pull request be closed now? It seems to me that publishing the docs could be dealt with separately. |
Fine by me. |
As discussed at #123 I worked on a new documentation system, still based on markdown format, which can then be used to generate html documentation (either locally using mkdocs or using 3rd party service like readthedocs). For simplicity I split the main README.md file into various docs/*.md file, and only kept the synopsis and a TOC with link in the README.
I split the various steps to make it easier to follow, the only interesting points IMHO are:
I choose to manually duplicate the TOC tree in the readme as having an automatic one isn't supported by default and I'd rather avoid relying on too many extensions.
As an example I temporarily configured https://pg-hint-plan.readthedocs.io/en/latest/ to point to this branch so you can check the generated html. The locally generated doc is strictly identical.
Let me know if you have any comments or modification you'd like to see. Also, should I backpatch this documentation change, and if yes how far? AFAICS the README.md was created in the pg14 version so it could be a good first step. If backpatch is wanted I will also make sure that RTD can handle that (it should).