Skip to content

Always unlink first segment for AO/AOCO temp relation.#1656

Open
reshke wants to merge 1 commit intomainfrom
unlink_temp_first_seg
Open

Always unlink first segment for AO/AOCO temp relation.#1656
reshke wants to merge 1 commit intomainfrom
unlink_temp_first_seg

Conversation

@reshke
Copy link
Copy Markdown
Contributor

@reshke reshke commented Apr 1, 2026

Usually relfiles from deleted relation persist until next CHECKPOINT, and unlinked only then. For temporary relation. this is not the case, and this is explicitly handled in mdunlinkfork

https://github.com/postgres/postgres/blob/868825a/src/backend/storage/smgr/md.c#L384-L385

With GPDB 6 -> 7 refactorings we ended up using separate unlink functions for relation obsoletion. In gpdb 6 we used to call mdunlink_ao from mdunlinkfork while in gpdb 7 we are not. This leads to orphan files after temporal relation drop like in scenario:

create tablespace  t location '/tmp/tts';
create temp table t (i int) using ao_row tablespace t distributed by (i);
drop table t;

Fix this by importing logic from mdunlinkfork

#1644

*/
if (RelFileNodeBackendIsTemp(rnode))
{
/* Next unlink the file, unless it was already found to be missing */
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.

Thanks for catching this!

Minor comments below:

int ret; is declared at function scope.
Better to be scoped inside the if block, or removed entirely since it's only checked against < 0 immediately.

(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
}

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.

Should also unlink the segment files beyond segment 0?

How about modifying mdunlink_ao_base_relfile() and mdunlink_ao_perFile() to check RelFileNodeBackendIsTemp(rnode) and unlink immediately (mirroring the heap mdunlinkfork pattern), rather than appending a separate unlink at the end of mdunlink_ao?

Something like

  if (!unlinkFiles->isRedo)
  {
      if (RelFileNodeBackendIsTemp(unlinkFiles->rnode))
      {
          ret = unlink(baserel);
          if (ret < 0 && errno != ENOENT)
              ereport(WARNING, ...);
      }
      else
      {
          // Existing truncate + deferred unlink logic
          ...
      }
  }

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.

2 participants