Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRewrites and reorganizes the FDW documentation into Prerequisites, Set up, Usage, and Drop server; adds explicit limitations and security notes (localhost/HBA/fdw.allow_local), step-by-step SQL examples for server/user mappings/foreign tables, explain output examples, and expanded cleanup/reference guidance. (47 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Moved an updated FDW page we had in gitbook to the Ingestion section with minor updates.
Thank you for any updates. However, please don't relocate: FDW has far more applications than just "ingestion". If you want to have it mentioned more prominently in this section, please tease and cross-link accordingly.
Fair enough, we can get back to that part later. |
bmunkholm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/feature/fdw/index.md`:
- Around line 69-92: The SQL example uses remote_readings but the foreign table
was created as doc.remote_readings; update the query example to use the
fully-qualified table name doc.remote_readings (or add a short note that the doc
schema must be in the search_path) so the example will work as-is; locate the
example query block that references remote_readings and replace it with
doc.remote_readings (or add the search_path note directly above the query).
🧹 Nitpick comments (1)
docs/feature/fdw/index.md (1)
57-62: Consider a short note about credentials handling.The example inlines
password '*****'. For docs, it may be worth noting whether secrets should be managed via secure settings or tooling to avoid embedding credentials in plain text.
Co-authored-by: Andreas Motl <andreas.motl@crate.io>
Co-authored-by: Andreas Motl <andreas.motl@crate.io>
|
(What an insane amount of timeouts we have in the build (https://github.com/crate/cratedb-guide/actions/runs/21951807340/job/63404431908?pr=548). |
Oh. Either it's bad Internet weather today, or SREs are ramping up countermeasures against rogue web crawlers. This will certainly make legitimate link checkers have bad times going forward. |
I'll give them some time then. Thanks for all the feedback! (Build errors are once again about rate-limiting in the link-checks. Addressed in theme.) |
| :::{rubric} System Tables | ||
| ### Set firewall rules | ||
|
|
||
| Ensure outbound firewall rules allow CrateDB → remote DB traffic before |
There was a problem hiding this comment.
Do we need to mention which TCP port (5432) needs to be open?
There was a problem hiding this comment.
we could as an example, but 5432 is not strict, just the default port.
There was a problem hiding this comment.
I wonder what value this content will bring as it seems to be roughly a 1:1 copy of the FDW reference documentation. Without adding any additional content (if needed at all), I'd suggest to keep it as-is (what was the issue here?) or link to the reference content more prominent.
If there is something to add to the existing reference documentation, please improve it instead of creating copies.
Thats a very reasonable concern. And it's valid for a LOT of the pages we have. We haven't come to reorganizing the Reference yet. But if it was up to me, and time enough was available, we would reorganize it significantly. Right now there is way too much overlap across many pages. But this still fits the current direction, and having multiple pages "under construction" in the "All Features" section doesn't give much confidence in the state of the docs tbh. Right now the FDW in Reference is burried under "Administration" which seems odd btw. In flow with the current direction, I would still suggest to add this. If we are to not duplicate info at all, we need another strategic direction and can delete a lot of pages. (which I totally would support - we just don't have the capacity to do it). |
seut
left a comment
seut
left a comment
There was a problem hiding this comment.
Thats a very reasonable concern. And it's valid for a LOT of the pages we have.
I didn't write the content here itself - I only populated it from Gitbook. The original thought was that in general a lot of the content in Reference is very detailed, and something more easy to get started with and organized for easy discovery was needed.
Only because this was done in the past, does not justify to me to keep up with this. Also, please judge by case, as this is really a 1:1 copy, no further guidance is added here, afaik.
Copying content makes everything more complex to maintain for no good reason, so let us stop here please.
If we really need to include the same content completely in different places, we should imho look into different solution like including remote content, e.g. by https://pypi.org/project/sphinxext-remoteliteralinclude/.
|
IMHO, the current page needs a make-over. If we don't want to gather documentation about FDW in one page, I suggest that we simplify the page to mention that the feature exists and link to the two following pages:
By simplifying, I mean removing everything from "Synopsis" and down, and remove "SQL functions" and "System tables". |
I absolutely agree. Currently, it is obviously just a stub. While I was happy that someone would fill the gaps, it doesn't make sense to add redundant content, which likely has been created by AI-based summarization tools instead of conceiving unique content. If this is the case here, I will come up with a different proposal as we go. |
In this case it was actually a human :-). |
|
I'd like to apologize if it was a human in this case, I don't want to hurt anyones feelings. We've lost a bit track, and also GH-264 did not make much progress 1, so we thought actually improving content was mostly out of scope, because it was merely a rehash of existing material in a different structure. After this converged progressively, it became more difficult to pick any cherries. Footnotes
|
There was a problem hiding this comment.
Hi again. I revisited the review because I didn't do a thorough content comparison before, and because @seut had admonitions about it, also content-wise. I've identified the primary offending section now, and I think the patch will be good to go if we just remove it and more prominently refer to the reference documentation page instead. Thanks!
I think the excellent stepper-based presentation of a typical FDW workflow is absolutely the right choice for this kind of page within the "all features" section that aims to provide an optimal layer above the reference documentation, to improve on guidance matters. It would be sad to leave this behind.
| Query clauses like `GROUP BY`, `HAVING`, `LIMIT` or `ORDER BY` are executed | ||
| within CrateDB, not within the foreign system. `WHERE` clauses can in some | ||
| circumstances be pushed to the foreign system, but that depends on the | ||
| concrete foreign data wrapper implementation. You can check if this is the | ||
| case by using the {ref}`crate-reference:ref-explain` statement. | ||
|
|
||
| For example, in the following explain output there is a dedicated `Filter` | ||
| node, indicating that the filter is executed within CrateDB: | ||
|
|
||
| ```sql | ||
| CREATE FOREIGN TABLE doc.remote_documents (name text) | ||
| SERVER my_postgresql | ||
| OPTIONS (schema_name 'public', table_name 'documents'); | ||
| EXPLAIN SELECT ts, value FROM remote_readings WHERE device = 'sensor-42'; | ||
| ``` | ||
|
|
||
| ```text | ||
| +--------------------------------------------------------------------------+ | ||
| | QUERY PLAN | | ||
| +--------------------------------------------------------------------------+ | ||
| | Filter[(device = 'sensor-42')] (rows=0) | | ||
| | └ ForeignCollect[doc.remote_readings | [device] | true] (rows=unknown) | | ||
| +--------------------------------------------------------------------------+ | ||
| ``` |
There was a problem hiding this comment.
It looks like this is the primary offending section with a 100% redundancy to the reference documentation page. Can you just remove it?
There was a problem hiding this comment.
Thanks! I'm not entirely sure what exactly you mean should be removed. The whole diff? (92-112)?
Instead of back and forth with me just acting as a scribe, perhaps it's easiest if you just edit the way you can see it being good? 🙏
There was a problem hiding this comment.
It is this original content I am referring to. It seems to have been copied 1:1 to that spot?
cratedb-guide/docs/feature/fdw/index.md
Lines 92 to 112 in f90aa90
There was a problem hiding this comment.
Aha. I see now that the SQL statement is different to probably match the tutorial, and that the paragraph above is the teaser to that, which is probably needed to make sense. Well, it still bears 95% redundancy otherwise, so I'd still vote to remove the section to satisfy corresponding admonitions from colleagues.
|
I put this back to draft as it's not ready for merging. @amotl If you have an exact idea about where to take this, I would appreciate if you do the edits. |
5 participants
About
Updated FDW page we had in gitbook with minor updates.
Fixes https://github.com/crate/tech-content/issues/151
Preview
https://cratedb-guide--548.org.readthedocs.build/feature/fdw/