chore: add docs with analysis on the open issues

Author: andinux

plans/ISSUE_POSTGRES_SCHEMA.md

@@ -0,0 +1,73 @@
+Issue summary
+
+cloudsync_init('users') fails in Supabase postgres with:
+"column reference \"id\" is ambiguous".
+Both public.users and auth.users exist. Several PostgreSQL SQL templates use only table_name (no schema), so information_schema lookups and dynamic SQL see multiple tables and generate ambiguous column references.
+
+Proposed fixes (options)
+
+1) Minimal fix (patch specific templates)
+- Add table_schema = current_schema() to information_schema queries.
+- Keep relying on search_path.
+- Resolves Supabase default postgres collisions without changing the API.
+
+2) Robust fix (explicit schema support)
+- Allow schema-qualified inputs, e.g. cloudsync_init('public.users').
+- Parse schema/table and propagate through query builders.
+- Always generate fully-qualified table names ("schema"."table").
+- Apply schema-aware filters in information_schema queries.
+- Removes ambiguity regardless of search_path or duplicate table names across schemas.
+- Note: payload compatibility requires cloudsync_changes.tbl to remain unqualified; PG apply should resolve schema via cloudsync_table_settings (not search_path) when applying payloads.
+
+Bugged query templates
+
+Already fixed:
+- SQL_PRAGMA_TABLEINFO_PK_COLLIST
+- SQL_PRAGMA_TABLEINFO_PK_DECODE_SELECTLIST
+
+Still vulnerable (missing schema filter):
+- SQL_BUILD_SELECT_NONPK_COLS_BY_ROWID
+- SQL_PRAGMA_TABLEINFO_LIST_NONPK_NAME_CID
+- SQL_CLOUDSYNC_DELETE_COLS_NOT_IN_SCHEMA_OR_PKCOL
+- SQL_PRAGMA_TABLEINFO_PK_QUALIFIED_COLLIST_FMT
+
+Robust fix implementation plan
+
+Goals
+- Support cloudsync_init('users') and cloudsync_init('public.users')
+- Default schema to current_schema() when not provided
+- Persist schema so future connections are independent of search_path
+- Generate fully qualified table names in all PostgreSQL SQL builders
+
+1) Parse schema/table at init
+- In cloudsync_init_table() (cloudsync.c), parse the input table_name:
+  - If it contains a dot, split schema/table
+  - Else schema = current_schema() (query once)
+- Normalize case to match existing behavior
+
+2) Persist schema in settings
+- Store schema in cloudsync_table_settings using key='schema'
+- Keep tbl_name as unqualified table name
+- On first run, if schema is not stored, write it
+
+3) Store schema in context
+- Add char *schema to cloudsync_table_context
+- Populate on table creation and when reloading from settings
+- Use schema when building SQL
+
+4) Restore schema on new connections
+- During context rebuild, read schema from cloudsync_table_settings
+- If missing, fallback to current_schema(), optionally persist it
+
+5) Qualify SQL everywhere (Postgres)
+- Use "schema"."table" in generated SQL
+- Add table_schema filters to information_schema queries:
+  - SQL_BUILD_SELECT_NONPK_COLS_BY_ROWID
+  - SQL_PRAGMA_TABLEINFO_LIST_NONPK_NAME_CID
+  - SQL_CLOUDSYNC_DELETE_COLS_NOT_IN_SCHEMA_OR_PKCOL
+  - SQL_PRAGMA_TABLEINFO_PK_QUALIFIED_COLLIST_FMT
+  - Any other information_schema templates using only table_name
+
+6) Compatibility
+- Existing DBs without schema setting continue to work via current_schema()
+- No API changes required for unqualified names

plans/ISSUE_WARNING_resource_was_not_closed.md

@@ -0,0 +1,60 @@
+# WARNING: resource was not closed: relation "cloudsync_changes"
+
+## Summary
+The warning was emitted by PostgreSQL when a SPI query left a “relation” resource open. In practice, it means a SPI tuptable (or a relation opened internally by SPI when executing a query) wasn’t released before the outer SQL statement completed. PostgreSQL 17 is stricter about reporting this, so the same issue might have been silent in earlier versions.
+
+We isolated the warning to the `cloudsync_payload_apply` path when it inserted into the `cloudsync_changes` view and triggered `cloudsync_changes_insert_trigger`. The warnings did **not** occur for direct, manual `INSERT INTO cloudsync_changes ...` statements issued in psql.
+
+## Why it only happened in the payload-apply path
+The key difference was **nested SPI usage** and **statement lifetime**:
+
+1. **`cloudsync_payload_apply` loops many changes and uses SPI internally**
+   - `cloudsync_payload_apply` is a C function that processes a payload by decoding multiple changes and applying them in a loop.
+   - For each change, it executed an `INSERT INTO cloudsync_changes (...)` (via `SQL_CHANGES_INSERT_ROW`), which fires the INSTEAD OF trigger (`cloudsync_changes_insert_trigger`).
+
+2. **The trigger itself executed SPI queries**
+   - The trigger function uses SPI to read and write metadata tables.
+   - This creates *nested* SPI usage within a call stack that is already inside a SPI-driven C function.
+
+3. **Nested SPI + `INSERT INTO view` has different resource lifetime than a plain insert**
+   - With a manual psql statement, the SPI usage occurs only once, in a clean top-level context. The statement finishes, SPI cleanup happens, and any tuptable resources are released.
+   - In the payload apply path, SPI queries happen inside the trigger, inside another SPI-driven C function, inside a loop. If any intermediate SPI tuptable or relation is not freed, it can “leak” out of the trigger scope and be reported when the outer statement completes.
+   - That’s why the warning appears specifically when the trigger is executed as part of `cloudsync_payload_apply` but not for direct inserts from psql.
+
+4. **PostgreSQL 17 reports this more aggressively**
+   - Earlier versions often tolerated missing `SPI_freetuptable()` calls without warning. PG17 emits the warning when the statement finishes and resources are still registered as open.
+
+## Why direct INSERTs from psql didn’t warn
+The smoke test included a manual `INSERT INTO cloudsync_changes ...`, and it never produced the warning. That statement:
+
+- Runs as a single SQL statement initiated by the client.
+- Executes the trigger in a clean SPI call stack with no nested SPI calls.
+- Completes quickly, and the SPI context is unwound immediately, which can mask missing frees.
+
+In contrast, the payload-apply path:
+
+- Opens SPI state for the duration of the payload apply loop.
+- Executes many trigger invocations before returning.
+- Accumulates any unfreed resources over several calls.
+
+So the leak only becomes visible in the payload-apply loop.
+
+## Fix that removed the warning
+We introduced a new SQL function that bypasses the trigger and does the work directly:
+
+- Added `cloudsync_changes_apply(...)` and rewired `SQL_CHANGES_INSERT_ROW` to call it via:
+  ```sql
+  SELECT cloudsync_changes_apply(...)
+  ```
+- The apply function executes the same logic but without inserting into the view and firing the INSTEAD OF trigger.
+- This removes the nested SPI + trigger path for the payload apply loop.
+
+Additionally, we tightened SPI cleanup in multiple functions by ensuring `SPI_freetuptable(SPI_tuptable)` is called after `SPI_execute`/`SPI_execute_plan` calls where needed.
+
+## Takeaway
+The warning was not tied to the `cloudsync_changes` view itself, but to **nested SPI contexts and missing SPI cleanup** during payload apply. It was only visible when:
+
+- the apply loop executed many insert-trigger calls, and
+- the server (PG17) reported unclosed relation resources at statement end.
+
+By switching to `cloudsync_changes_apply(...)` and tightening SPI tuptable cleanup, we removed the warning from the payload-apply path while leaving manual insert behavior unchanged.