Revert index scan #247
Revert index scan #247
Conversation
| @@ -36,7 +35,7 @@ PostgresTable::PopulateColumns(CreateTableInfo &info, Oid relid, Snapshot snapsh | |||
| auto tupleDesc = RelationGetDescr(rel); | |||
|
|
|||
| if (!tupleDesc) { | |||
| elog(ERROR, "Failed to get tuple descriptor for relation with OID %u", relid); | |||
There was a problem hiding this comment.
I think this is probably a good change, but let's move it to a separate PR. It's nice to keep behavioral and refactoring changes separate, both for review and possible future git revert.
|
|
||
| private: | ||
| Path *path; | ||
| PlannerInfo *planner_info; |
There was a problem hiding this comment.
Do we still need the PlannerInfo to be passed into DuckDB now at all? i.e. I think we were only using it to choose between sequence scan and index scan, so I think we can get rid of storing it in the "postgres_state" key of context->registered_state.
There was a problem hiding this comment.
I think you're right, I think that might also remove the entire need for PostgresContextState, I believe m_rtables and m_needed_columns as well as m_query_string are not used ?
There was a problem hiding this comment.
If that's indeed the case, let's remove those too. That simplifies stuff a lot.
There was a problem hiding this comment.
I'm also fine with doing that in a follow up PR btw.
mkaruza
force-pushed
the
revert-index-scans
branch
2 times, most recently
from
e0b0b14 to
5ac446b
Compare
src/catalog/pgduckdb_table.cpp
Outdated
| duck_type.ToString().c_str()); | ||
| } | ||
|
|
||
| RelationClose(rel); | ||
| return true; | ||
| } | ||
|
|
||
| Cardinality | ||
| PostgresTable::GetTableCardinality(Oid relid) { | ||
| auto rel = RelationIdGetRelation(relid); |
There was a problem hiding this comment.
I think this probably needs the DuckdbProcessLock.
There was a problem hiding this comment.
Let me give another pass through code; I'm seeing this RelationIdGetRelation used on few other places (unprotected by lock) maybe it is worth checking if this function can be used in only one function and than we would pass pointer to its data.
There was a problem hiding this comment.
@Tishj can you look into latest commit it should centralize opening/closing relation for execution
mkaruza
force-pushed
the
revert-index-scans
branch
from
5ac446b to
03538ce
Compare
src/catalog/pgduckdb_table.cpp
Outdated
| auto tupleDesc = RelationGetDescr(rel); | ||
|
|
||
| if (!tupleDesc) { | ||
| elog(WARNING, "Failed to get tuple descriptor for relation with OID %u", relid); | ||
| elog(WARNING, "Failed to get tuple descriptor for relation with OID %u", rel->rd_id); | ||
| RelationClose(rel); |
There was a problem hiding this comment.
This one should now be removed I think.
There was a problem hiding this comment.
Yes, thanks. I'm thinking if this check is even needed? If we have relation object opened before should we always have tuple desc information?
There was a problem hiding this comment.
Yeah, that sounds reasonable. There's no way for this to fail. It's a simple macro that reads a field:
#define RelationGetDescr(relation) ((relation)->rd_att)
|
Oh one more small note (I forgot it last time too when merging a PR without squashing). Can you update the commit messages to include the PR number at the end of the title in parenthesis, ie. |
Reverting index scan feature as explained in #183.
By removing index scan we don't need anymore to do query planning. Remove this from execution. Table cardinality is directly calculated by calling callback functions.
Opening and closing relation is not thread safe. So it would be better that we open and close relation from central point and pass pointer to structure for reading.
mkaruza
force-pushed
the
revert-index-scans
branch
from
03538ce to
b319fa1
Compare
Reverting index scan feature as explained in #183. Removed query planning from execution.
Three commits that should not be squashed when merging.
Fixes #183